jenkins-infra / jenkins-io-components

https://jenkins-io-components.netlify.app
MIT License
4 stars 25 forks source link

fix(footer): Clean up Footer styling #54

Closed Vandit1604 closed 1 year ago

Vandit1604 commented 1 year ago

closes https://github.com/jenkins-infra/jenkins.io/issues/5922

Vandit1604 commented 1 year ago

@halkeye Can you guide me about why tests are failing ? Do i have to know something about chromatic to understand the failure ?

halkeye commented 1 year ago

chromatic is fine, its just complaining its a fork. Its a non blocking check so won't be an issue. Tests i'm not sure whats going on. I don't know enough about github actions. Might have to move it to ci.jenkins (it was github actions when prototyping)

Vandit1604 commented 1 year ago
›   Error: Timed out waiting for authorization. If you do not have a Netlify account, please create one at https://app.netlify.com/signup, then run netlify login again.

it saying this i have an netlify account there's no deploy there. its confusing

halkeye commented 1 year ago

it saying this i have an netlify account there's no deploy there. its confusing

don't worry about it, its something misconfigured on the repo, it'll probably take me some time to get to it.

Vandit1604 commented 1 year ago

If i can be of any help let me know please

halkeye commented 1 year ago

https://deploy-preview-54--jenkins-io-components.netlify.app/?path=/story/example-footer--has-source-path @MarkEWaite @kmartens27 does that look good to you?

Looking at the original ticket, @Susmita-Dey is suggesting the "Creative Commons" word be bold, not the entire license be bold. I don't find a paragraph of bolded text be very visually appealing so i'm not a fan.

Also the license name is "Creative Commons Attribution-ShareAlike 4.0" not "Creative Commons" so i'm not sure what bolding part of it would gain.

Vandit1604 commented 1 year ago

Also the license name is "Creative Commons Attribution-ShareAlike 4.0" not "Creative Commons" so i'm not sure what bolding part of it would gain.

I don't think making only the words "Creative Commons" when the complete license is "Creative Commons Attribution-ShareAlike 4.0". Should i go with making only Creative Commons Attribution-ShareAlike 4.0 Bold

halkeye commented 1 year ago

Should i go with making only Creative Commons Attribution-ShareAlike 4.0 Bold

I don't like leaving things unanswered but I'm not ignoring you, i just don't know, you'll have to wait for the others

MarkEWaite commented 1 year ago

Also the license name is "Creative Commons Attribution-ShareAlike 4.0" not "Creative Commons" so i'm not sure what bolding part of it would gain.

I don't think making only the words "Creative Commons" when the complete license is "Creative Commons Attribution-ShareAlike 4.0". Should i go with making only Creative Commons Attribution-ShareAlike 4.0 Bold

I disagree with the premise of the original issue report that says that the license should be made bold. Bold text is reserved for things that need the attention of the reader. The license agreement does not need their attention. As further support, the Debian project, the FreeBSD project, the Fedora project, the Ubuntu project, and the Python project do not bold their license information or the links to their license information and legal information. I found only one example of bold text, and that was purely in the headings of the FreeBSD project license details page.

kmartens27 commented 1 year ago

Seconding @MarkEWaite's comment, this text does not need to be bold, and can be left as regular text.

halkeye commented 1 year ago

so your description is "Increase spacing" but now it looks like jio-improve-this-page and jio-report-issue are on seperate lines.

are you thinking you want p.box not p .box? you could also do jio-improve-this-page { margin-right: 10px; } or whatever also

I recommend you test this locally with npm run dev

Susmita-Dey commented 1 year ago

https://deploy-preview-54--jenkins-io-components.netlify.app/?path=/story/example-footer--has-source-path @MarkEWaite @kmartens27 does that look good to you?

Looking at the original ticket, @Susmita-Dey is suggesting the "Creative Commons" word be bold, not the entire license be bold. I don't find a paragraph of bolded text be very visually appealing so i'm not a fan.

Also the license name is "Creative Commons Attribution-ShareAlike 4.0" not "Creative Commons" so i'm not sure what bolding part of it would gain.

My idea was to make "Creative Commons Attribution-ShareAlike 4.0" bold like:- Creative Commons Attribution-ShareAlike 4.0

Vandit1604 commented 1 year ago

so your description is "Increase spacing" but now it looks like jio-improve-this-page and jio-report-issue are on seperate lines.

Hey @halkeye i didn't quite understand what you meant as in the deployment its still on the same line. Can you elaborate on that.

are you thinking you want p.box not p .box?

Also i have used p .box not p.box

I recommend you test this locally with npm run dev

Okay i'll keep that in mind thanks for your time

Vandit1604 commented 1 year ago

Does infra.ci.jenkins.io takes some time before showing details of failure?

MarkEWaite commented 1 year ago

Does infra.ci.jenkins.io takes some time before showing details of failure?

See https://status.jenkins.io/issues/2023-01-19-infra-ci-migration/ for the announcement that infra.ci.jenkins.io is currently down so that it can migrate to a new Kubernetes cluster.

Vandit1604 commented 1 year ago

Thanks @MarkEWaite That explains. I'll solve the failures after few hours then.

lemeurherve commented 1 year ago

@Vandit1604 I've restarted a build since the infra.ci.jenkins.io migration has been completed, here is the current lint error:

[2023-01-19T18:34:42.606Z] + npx eslint --format checkstyle .
[2023-01-19T18:34:45.125Z] + npx stylelint --custom-formatter ./node_modules/stylelint-checkstyle-formatter 'src/**/*.css' -o stylelint-results.json
[2023-01-19T18:34:46.052Z] <?xml version="1.0" encoding="utf-8"?>
[2023-01-19T18:34:46.052Z] <checkstyle version="4.3">
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/col.css"></file>
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/container.css"></file>
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/global.css"></file>
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/jio-footer.css">
[2023-01-19T18:34:46.052Z]     <error source="stylelint.rules.prettier/prettier" line="90" column="2" severity="error" message="Insert &quot;⏎&quot; (prettier/prettier)" />
[2023-01-19T18:34:46.052Z]   </file>
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/jio-navbar.css"></file>
[2023-01-19T18:34:46.052Z]   <file name="/home/jenkins/agent/workspace/jobs_jenkins-io-components_PR-54/src/jio-socialmedia-buttons.css"></file>
[2023-01-19T18:34:46.060Z] </checkstyle>
script returned exit code 2
halkeye commented 1 year ago

@lemeurherve do you know why PR 54 isn't on https://ci.jenkins.io/job/Websites/job/jenkins-io-components/view/change-requests/

Also I think we can do all the checks on ci.jenkins (public), we only need infra for netlify and release.

halkeye commented 1 year ago

Is this ready for a re-review?

1) image It looks like its on a new line, is that intention? i thought the plan was just to add a bit of a gap. 2) Looks like extra temp files got added to the PR (don't use git add . which adds all changed non-ignored files, if you want to do all changed files, you can do git add -p . which will give you a prompt for each change)

Vandit1604 commented 1 year ago

@halkeye In my PC its on the same line. Am i doing something different ?

Screenshot from 2023-01-20 03-15-21

halkeye commented 1 year ago

In my PC its on the same line. Am i doing something different ?

This the the big win for the preview site. It'll show you what is committed, and let other people see it. If i get chromatic working again, it should even let us know which components had ui changes. Maybe you have local changes you havn't committed. Check the files tab to confirm things are what you expect

https://deploy-preview-54--jenkins-io-components.netlify.app/?path=/story/example-footer--external-property

My screenshot above is in firefox. When i try it in chrome it looks even worse.

image

Looks like at least with the margin, its now too wide for the box.

Vandit1604 commented 1 year ago

I checked it on Firefox too. It's still on the same line and i also don't have uncommitted changes so what can be the reason for it to be on different lines on your PC Screenshot from 2023-01-20 03-37-40 :cry:

halkeye commented 1 year ago

image

Maybe its the font linux uses? If its wrapping because of text size, its probably too tight. Maybe reduce it a bit from 15? I'll try on windows too.

halkeye commented 1 year ago

yea on windows with Segoe UI it looks fine.

Vandit1604 commented 1 year ago

I'll reduce it to 10px maybe then it'll work

halkeye commented 1 year ago

looks like mine wraps at 7px.

I think your original plan of flex is a better plan, because it won't have that ugly indent in chrome I had in my previous screenshots.

halkeye commented 1 year ago

or maybe do margin right for improve, instead of left for report a problem.

Vandit1604 commented 1 year ago

Is it alright now ?

halkeye commented 1 year ago

Looks good on my firefox and chrome in linux at desktop widths.

Mobile widths with display: box makes the text wrap downwards

image

Without the display box, it wraps the entire element

image

I don't know which is better, I don't do much in the design type stuff.

halkeye commented 1 year ago

@jenkins-infra/copy-editors any opinions?

Vandit1604 commented 1 year ago

if both the links get in different lines on mobile width it'll good as right now on https://jenkins.io on smaller screens both link get in different lines

Screenshot_2023_0120_043958.jpg

Vandit1604 commented 1 year ago

Now the Improve this page and Report a problem links will get on different lines on smaller screens. Can you please check if it's working properly on other browsers too.

halkeye commented 1 year ago

kicking off a re-run. Still need to track down why this PR isn't showing up on https://ci.jenkins.io/job/Websites/job/jenkins-io-components/view/change-requests/ but infra.ci (vpn only) had a timeout.

Vandit1604 commented 1 year ago

Does everything look good on all browsers?

halkeye commented 1 year ago

I'm working so I can take a look when i can take a look

image Chrome the margin causes it to wrap.

I think its good enough. Would like @jenkins-infra/copy-editors on design changes though

MarkEWaite commented 1 year ago

I think its good enough. Would like @jenkins-infra/copy-editors on design changes though

@Stackscribe and I reviewed this during documentation office hours. We think it is a very nice improvement. We checked various widths and saw no issues at any of the widths. We think it is ready to ship.

jenkins-io-components[bot] commented 1 year ago

:tada: This PR is included in version 1.17.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: