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

Tweak sitenav design #2204

Merged
merged 13 commits into from
Mar 15, 2023
Merged

Tweak sitenav design #2204

merged 13 commits into from
Mar 15, 2023

Conversation

itsyme
Copy link
Contributor

@itsyme itsyme commented Mar 11, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Resolves #2085. Removed the bold in the site-nav in user guide. Updated the paddings to make the text vertically centered. Updated the font sizes of the different tiers (values were chosen by setting the first tier to an appropriate size and scaling the subsequent tiers by a factor of 1.16 (golden ratio) down from the previous tier).

Anything you'd like to highlight/discuss:
Even after editing the design of site-nav-top, I still feel it can improved further. Seeking feedback/suggestions to improve site-nav-top or any other part of site-nav.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Tweak sitenav design


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt
Copy link
Contributor

tlylt commented Mar 12, 2023

Vertical align looks weird to me, I think original left align is fine. Otherwise LGTM

@itsyme
Copy link
Contributor Author

itsyme commented Mar 12, 2023

Hi @tlylt! I totally agree, I changed it back to be left aligned but increased the font size slightly. I initially centered it to try and differentiate the site-nav-top from the first item in the site-nav as I felt that it could be slightly confusing. I feel that this increase in font size would achieve a similar effect.

tlylt
tlylt previously approved these changes Mar 12, 2023
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM

@tlylt tlylt added this to the v4.1.1 milestone Mar 12, 2023
@itsyme
Copy link
Contributor Author

itsyme commented Mar 13, 2023

Hi @tlylt! The tiers after third tier of dropdowns seem to not have their font sizes scaled down. Would you think it's better to continue scaling it down further or to keep it at the smallest font size or to leave as is and write the limit in the user guide?
Screenshot 2023-03-13 at 4 40 42 PM

@tlylt
Copy link
Contributor

tlylt commented Mar 13, 2023

Hi @itsyme, I think it's ok to leave it as is. In the above image, it seems like the icons are not being aligned nicely due to the change in size. Wondering if this is introduced by this change and if we can do something about it?

@itsyme
Copy link
Contributor Author

itsyme commented Mar 13, 2023

Hi @itsyme, I think it's ok to leave it as is. In the above image, it seems like the icons are not being aligned nicely due to the change in size. Wondering if this is introduced by this change and if we can do something about it?

It was not introduced by this change but I think it made the difference bigger due to the larger difference in font size. The screenshot below is the previous behaviour. I will try to fix the alignment in the next commit to make the arrows center-aligned!

Screenshot 2023-03-13 at 7 42 33 PM

@tlylt tlylt dismissed their stale review March 13, 2023 13:53

pending update

@itsyme
Copy link
Contributor Author

itsyme commented Mar 13, 2023

Hi @tlylt! I managed to align them but I had to introduce a small transition to mask the solution. The issue was that it was using em for width which made the width of the button change as the font size decreased. I changed it to rem which fixed the issue. However, this introduced a new issue of the background becoming oval when the button is hovered. I added a transition to make the width back to em on hover. This creates a small transition that is rather aesthetic in my opinion. The larger buttons move less and the smaller buttons move more, creating a sense of inertia to the buttons. What do you think?

Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

LGTM @tlylt for the final say

@tlylt
Copy link
Contributor

tlylt commented Mar 14, 2023

Hi @tlylt! I managed to align them but I had to introduce a small transition to mask the solution. The issue was that it was using em for width which made the width of the button change as the font size decreased. I changed it to rem which fixed the issue. However, this introduced a new issue of the background becoming oval when the button is hovered. I added a transition to make the width back to em on hover. This creates a small transition that is rather aesthetic in my opinion. The larger buttons move less and the smaller buttons move more, creating a sense of inertia to the buttons. What do you think?

Thanks @itsyme, I tried the animation. I think sometimes at certain positions the transition feels like a shaking animation such that the button just shakes from left to right continuously. This is not very obvious and only occurs when I put my cursor near the edge of the button. I could not capture this.

Ping @MarkBind/active-devs if anyone has any opinion on the aesthetic of the transition. I find it slightly out of place but ok to add it.

docs/css/main.css Outdated Show resolved Hide resolved
@itsyme
Copy link
Contributor Author

itsyme commented Mar 14, 2023

Thanks @itsyme, I tried the animation. I think sometimes at certain positions the transition feels like a shaking animation such that the button just shakes from left to right continuously. This is not very obvious and only occurs when I put my cursor near the edge of the button. I could not capture this.

Thanks for testing it out @tlylt! I had another transition of just expanding the button (below). However, only the 0-th, 2-nd and 3-rd tier will expand as the 1-st tier has the same font size as the root, making it inconsistent. I added the movement to the right to make the transition consistent with all the buttons. What do you think?

Screen Recording 2023-03-14 at 9 22 01 AM

Copy link
Contributor

@jovyntls jovyntls left a comment

Choose a reason for hiding this comment

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

@tlylt is this what you were referring to?

Screen.Recording.2023-03-14.at.11.01.29.AM.mov

I would also prefer if the transition didn't make the button move towards the right since it feels like the button is moving away from my cursor (especially when my cursor approaches from the left). Personally I think a minimal aesthetic would be nice for the sitenav, but no strong opinions on this.

@tlylt
Copy link
Contributor

tlylt commented Mar 14, 2023

@tlylt is this what you were referring to?

I would also prefer if the transition didn't make the button move towards the right since it feels like the button is moving away from my cursor (especially when my cursor approaches from the left). Personally I think a minimal aesthetic would be nice for the sitenav, but no strong opinions on this.

Yes and agreed!

@itsyme
Copy link
Contributor Author

itsyme commented Mar 14, 2023

I tried to align them by calculating the margin by calc(1.5rem - 1.5em) but to no avail. Without the transition it appears as in the second bullet point below.

These are other possibilities we can explore:

  1. Without margin transition (tier 2 and 3 moves in different direction from tier 0 and 1):
Screen.Recording.2023-03-14.at.11.43.48.AM.mov
  1. Without any transition (background visibly rotates with icon):
Screen.Recording.2023-03-14.at.11.47.38.AM.mov
  1. Move to the right instead:
Screen.Recording.2023-03-14.at.11.50.40.AM.mov

Do let me know your thoughts!

@damithc
Copy link
Contributor

damithc commented Mar 14, 2023

Be careful with using rotation to direct attention, as the direction of the chevron has a meaning.

@itsyme
Copy link
Contributor Author

itsyme commented Mar 15, 2023

Hi @tlylt! I managed to fix the button alignments successfully by wrapping them in a div. There is one more potential change that could improve consistency of site-nav. When the scrollbar appears because the contents exceed the height of site-nav, it causes the dropdown buttons to move to the right. This existed even before this PR. Do you think this is an issue?

Screen.Recording.2023-03-15.at.8.16.21.AM.mov

@tlylt
Copy link
Contributor

tlylt commented Mar 15, 2023

Hi @tlylt! I managed to fix the button alignments successfully by wrapping them in a div. There is one more potential change that could improve consistency of site-nav. When the scrollbar appears because the contents exceed the height of site-nav, it causes the dropdown buttons to move to the right. This existed even before this PR. Do you think this is an issue?

Good point, I think it would be great if there are some space allocated for the scrollbar such that its appearance and disappearance does not shift the layout. If it's easy enough to implement then pls feel free to add it in this PR. A separate PR would be fine as well.

@itsyme
Copy link
Contributor Author

itsyme commented Mar 15, 2023

Hi @tlylt! I managed to fix the button alignments successfully by wrapping them in a div. There is one more potential change that could improve consistency of site-nav. When the scrollbar appears because the contents exceed the height of site-nav, it causes the dropdown buttons to move to the right. This existed even before this PR. Do you think this is an issue?

Good point, I think it would be great if there are some space allocated for the scrollbar such that its appearance and disappearance does not shift the layout. If it's easy enough to implement then pls feel free to add it in this PR. A separate PR would be fine as well.

I think I would open a separate PR for this in the future as the solution I have in mind could be a breaking change.

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @itsyme. For the breaking changes let me confirm again and we can think about adding them in if we decide to do a major version bump.

Can you help me:

  • file the issue on the scrollbar
  • update this branch with latest master to avoid any potential conflicts since I just merged another PR dealing with test files changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve site nav coloring/font schemes
5 participants