Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix varies of style issues for community page #43740

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

nekomeowww
Copy link
Contributor

@nekomeowww nekomeowww commented Oct 30, 2023

Changes

  1. Fixed overflowed line through of headings, use a better solution to achieve the same effect
  2. Adjusted the font size of headings when in smaller screens, it was way too big for mobile devices and reduces readability
  3. Added paddings for the Recent News section, it used to have no paddings and looks weird
  4. Adjusted the navigation anchor links for sections, the button style was way too big and not harmony with the rest of the design pattern
  5. Added comments for blocks
  6. Make it more consistent for paddings, margins and other styles for each of the sections
  7. Fixed the entangled styles with the size of view ports, make it more smoother when resizes
  8. Remove hard-coded paddings and margins
  9. Fixed incorrect vw unit usages, center navigation items, linted function calls
  10. Added some small animation transitions for the smoother experience

Pull Request Dependency

Requires

Previous

Desktop

Inconsistent Padding

image image

Mobile

Height of banner image is too small

image

Lines overflow

image

Inconsistent Padding

image

After

Desktop

image image

Mobile

image image image image

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Oct 30, 2023
@netlify
Copy link

netlify bot commented Oct 30, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 31dc056
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/656ed2ce80c9ba00082cefff
😎 Deploy Preview https://deploy-preview-43740--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dipesh-rawat
Copy link
Member

/area web-development

@k8s-ci-robot k8s-ci-robot added the area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes label Oct 30, 2023
Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the quick look of the Preview Page, this looks like an improvement!
LGTM

Preview Community Page

Copy link
Member

@Gauravpadam Gauravpadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

A significant improvement! Thanks for this PR

@Gauravpadam
Copy link
Member

@nekomeowww while we're at it, I can see some more improvements we can make (only if you wish to make them)

  • The navigation items can be centred (with or without media queries)
  • There's some persistent horizontal scrolling, If you can possibly target that?

Rest all good, Thanks again

@nekomeowww
Copy link
Contributor Author

@nekomeowww while we're at it, I can see some more improvements we can make (only if you wish to make them)

  • The navigation items can be centred (with or without media queries)

  • There's some persistent horizontal scrolling, If you can possibly target that?

Rest all good, Thanks again

Absolutely! Although, it's a bit late for tonight in my time zone, I will tweak it and get it squashed and updated tomorrow ASAP.

@nekomeowww
Copy link
Contributor Author

nekomeowww commented Oct 31, 2023

The navigation items can be centred (with or without media queries)

image

Is this the navigation items you talked about? What do you mean by "can be centered"? It is now centered on screens except mobile devices, it will left align the navigation items on left. Are you saying you want to center these navigation items even on mobile devices?

There's some persistent horizontal scrolling, If you can possibly target that?

@Gauravpadam Can you provide me some screenshots or steps to reproduce? On what size of the device are you experiencing such horizontal scroll issue?

@nekomeowww nekomeowww force-pushed the dev/docs-community-styles branch from ed09083 to 462315a Compare October 31, 2023 04:26
@Gauravpadam
Copy link
Member

Gauravpadam commented Oct 31, 2023

I'm on a viewport of ~1600px width

Is this the navigation items you talked about?

Correct, In smaller viewports, Flex can act up and break the alignment as so
image

Perhaps some justify-content will help?

Can you provide me some screenshots or steps to reproduce?

image

@nekomeowww
Copy link
Contributor Author

@Gauravpadam

Perhaps some justify-content will help?

Yes. Adjusted.

Interesting. I've tested all sorts of the viewports all the way from 4K to 720P with or without refreshes, I never got the overflow scroll like you did.

Do you know how to use devtools to point the elements? May I ask you to help me to find the overflow element that causes the scroll?

@Gauravpadam
Copy link
Member

Do you know how to use devtools to point the elements? May I ask you to help me to find the overflow element that causes the scroll?

Sure, I'll try some tinkering and get back to you

Copy link
Member

@Gauravpadam Gauravpadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing these should solve our problems

static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Outdated Show resolved Hide resolved
static/css/community.css Show resolved Hide resolved
Copy link
Member

@Gauravpadam Gauravpadam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that fits!

LGTM, Thanks for the good work @nekomeowww
(You'll have to wait a bit more for an approver's approval though)

@sftim
Copy link
Contributor

sftim commented Oct 31, 2023

On a wide viewport, these changes make the menu list harder to read (very big gaps between items).

Before

Screenshot


After

Screenshot

however, I don't object if the changes are overall a benefit. What do other reviewers think?

@sftim
Copy link
Contributor

sftim commented Oct 31, 2023

Aside / doesn't block a merge
The X (aka Twitter) content is a lot to scroll through; if we can shrink that and still have it useful, even better.

@nekomeowww
Copy link
Contributor Author

nekomeowww commented Oct 31, 2023

On a wide viewport, these changes make the menu list harder to read (very big gaps between items).

@sftim I agree with you. I should keep the padding around navigation items to prevent it being to wide to read. I will adjust it as suggested and make it more closer to the previous design.

The X (aka Twitter) content is a lot to scroll through; if we can shrink that and still have it useful, even better.

Good point. I will try to find a suitable design to try to make it better to scroll so the footer won't be such meaningless.

@nekomeowww nekomeowww force-pushed the dev/docs-community-styles branch from 1f3a9cb to 6c1cd08 Compare October 31, 2023 14:26
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 31, 2023
@nekomeowww nekomeowww closed this Nov 17, 2023
@nekomeowww nekomeowww force-pushed the dev/docs-community-styles branch from 6440875 to ee960b6 Compare November 17, 2023 03:25
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 17, 2023
@nekomeowww nekomeowww reopened this Nov 17, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 17, 2023
@nekomeowww
Copy link
Contributor Author

image

Please note this "Nothing to see here - yet" empty list for Twitter (X) widget wasn't broke by either the related #43766 Pull Request and this Pull Request.

It is something the developer platform of Twitter (X) team should address and get fixed.

About the "Nothing to see here - yet" issue of Twitter (X) widget, there are two forums that are actively being used by community to report the incidents, read more and follow here:

@nekomeowww
Copy link
Contributor Author

@sftim @Gauravpadam Could you please take a review on this?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@nekomeowww nekomeowww force-pushed the dev/docs-community-styles branch from 100f61a to 4ede9c5 Compare November 30, 2023 05:56
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023
Copy link
Member

@aj11anuj aj11anuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout break problem is already fixed for main website of this page even for smaller viewports. But all the changes and enhancements of this PR seems betterment of whole page. Just one thing, if you can increase the size of headings a bit for smaller viewports, then it will look better.

Screenshot_2023-12-05-06-57-44-869_com android chrome

@nekomeowww
Copy link
Contributor Author

Layout break problem is already fixed for main website of this page even for smaller viewports. But all the changes and enhancements of this PR seems betterment of whole page. Just one thing, if you can increase the size of headings a bit for smaller viewports, then it will look better.

Sure. No problem.

But which one does everyone prefer?

Use 24px

image

Use 28px

image

1. Fixed overflowed line through of headings, use a better solution to achieve the same effect
2. Adjusted the font size of headings when in smaller screens, it was way too big for mobile devices and reduces readability
3. Added paddings for the Recent News section, it used to have no paddings and looks weird
4. Adjusted the navigation anchor links for sections, the button style was way too big and not harmony with the rest of the design pattern
5. Added comments for blocks
6. Make it more consistent for paddings, margins and other styles for each of the sections
7. Fixed the entangled styles with the size of view ports, make it more smoother when resizes
8. Remove hard-coded paddings and margins
9. Fixed incorrect vw unit usages, center navigation items, linted function calls
10. Added some small animation transitions for the smoother experience

Signed-off-by: Neko Ayaka <neko@ayaka.moe>
@nekomeowww nekomeowww force-pushed the dev/docs-community-styles branch from 4ede9c5 to 31dc056 Compare December 5, 2023 07:35
Copy link
Member

@aj11anuj aj11anuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: fe6eb6a4eb3bdbdf8536e9b9567c559424febde5

@sftim
Copy link
Contributor

sftim commented Jan 7, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2024
@k8s-ci-robot k8s-ci-robot merged commit 7fd7300 into kubernetes:main Jan 7, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants