Skip to content

Localnav & Navigator UI changes and refactors #772

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

Merged
merged 57 commits into from
Feb 16, 2024

Conversation

marinaaisa
Copy link
Member

@marinaaisa marinaaisa commented Dec 22, 2023

Bug/issue #116401230, if applicable:

Summary

Localnav & Navigator UI changes and refactors:

  • Navigator's font size increased
  • UI Changes on Navigator's focus state
  • Localnav should take the entire width of the page
  • Integration of the Technology title into the Navigator
  • Breadcrumbs are moved into the Hero
  • Fix tests

Dependencies

NA

Testing

Steps:

  1. Run docc-render
  2. Assert that there are no regressions on the new UI changes that affect the localnav and navigator

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

@marinaaisa marinaaisa marked this pull request as ready for review January 2, 2024 16:41
@marinaaisa marinaaisa force-pushed the r116401230/localnav-ui-updates branch from 9b5e9e7 to e4a2fef Compare January 3, 2024 19:06
Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

Haven't had time to fully test/review this yet, but I'm noticing a couple of potential issues and I have some subjective feedback on some of the design changes.

Design feedback:

  • the increased font size in the navigator is a little jarring to me personally—it feels odd that it's so much bigger than the similar links in the Topics section on the actual page
  • the hierarchy/eyebrow/title area can be pretty crowded now depending on the content of the 3 items—not sure of a great alternative offhand, just something I'm noticing
  • the hover state for the hierarchy dropdown items seems a little off in dark mode, not sure if it was a color change or just a underline issue or what

Potential issues:

  • some refactoring may be accidentally affecting font sizes of other elements outside of the nav items, not sure if that was intended
  • I'm sometimes seeing JS errors in the console when using the nav filter—not exactly sure yet how to reproduce it reliably, but you may want to experiment with this and double check on that

I'd like to do some more review/testing but wanted to give you some initial feedback—I'm excited about the idea of cleaning up the nav elements and consolidating it in the way you've done here—very cool 😁

@marinaaisa
Copy link
Member Author

Haven't had time to fully test/review this yet, but I'm noticing a couple of potential issues and I have some subjective feedback on some of the design changes.

Design feedback:

  • the increased font size in the navigator is a little jarring to me personally—it feels odd that it's so much bigger than the similar links in the Topics section on the actual page
  • the hierarchy/eyebrow/title area can be pretty crowded now depending on the content of the 3 items—not sure of a great alternative offhand, just something I'm noticing
  • the hover state for the hierarchy dropdown items seems a little off in dark mode, not sure if it was a color change or just a underline issue or what

Thank you for the design feedback, @mportiz08 ! I'll sync with the design team about them.

Potential issues:

  • some refactoring may be accidentally affecting font sizes of other elements outside of the nav items, not sure if that was intended

Yes, I'm aware of it. I also need to test the tutorials pages more.

  • I'm sometimes seeing JS errors in the console when using the nav filter—not exactly sure yet how to reproduce it reliably, but you may want to experiment with this and double check on that

If you could help me to reproduce those errors, that would be fantastic!

I'd like to do some more review/testing but wanted to give you some initial feedback—I'm excited about the idea of cleaning up the nav elements and consolidating it in the way you've done here—very cool 😁

Thanks 😄

@marinaaisa marinaaisa force-pushed the r116401230/localnav-ui-updates branch from 3241235 to f189ed8 Compare January 5, 2024 16:12
@marinaaisa
Copy link
Member Author

@swift-ci test

@marinaaisa marinaaisa requested a review from mportiz08 January 5, 2024 16:31
Copy link
Contributor

@hqhhuang hqhhuang left a comment

Choose a reason for hiding this comment

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

This is looking great! A few thoughts:

  1. What's the reasoning behind moving the nav to the hero? I think it's alright in wide viewports, but quite confusing in the small viewport, as the nav dropdown now only shows the language switcher and API changes (when available) and doesn't provide actual nav information. It's also a bit hard to read in the hero, since the font blends in with the rest of the text.
image
  1. Can we make the name of the technology more obviously clickable in the nav bar? It doesn't seem clickable to me until I started scrolling. Because we removed the bottom border, it is also a bit jarring that that piece of the UI is sticky when I scroll within the navigator. The name of the technology is also no longer sticky when I scroll down on the main documentation page, which I think is a regression we should avoid.
image

@marinaaisa marinaaisa requested a review from hqhhuang January 10, 2024 15:39
@marinaaisa marinaaisa requested a review from hqhhuang February 8, 2024 16:27
@marinaaisa
Copy link
Member Author

@swift-ci test

Copy link
Contributor

@hqhhuang hqhhuang left a comment

Choose a reason for hiding this comment

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

Found a few small things. Almost there!! 🎉

@marinaaisa marinaaisa requested a review from hqhhuang February 12, 2024 14:11
@marinaaisa marinaaisa force-pushed the r116401230/localnav-ui-updates branch from 2eac1e0 to 4174106 Compare February 13, 2024 19:22
@marinaaisa marinaaisa force-pushed the r116401230/localnav-ui-updates branch from 4174106 to 29b7d4a Compare February 13, 2024 20:33
@marinaaisa
Copy link
Member Author

@swift-ci test

Copy link
Contributor

@hqhhuang hqhhuang left a comment

Choose a reason for hiding this comment

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

Just 1 more minor comment about displaying hierarchy in IDE mode.

@marinaaisa marinaaisa requested a review from hqhhuang February 15, 2024 18:18
@marinaaisa
Copy link
Member Author

Just 1 more minor comment about displaying hierarchy in IDE mode.

Answered :)

Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

LGTM

Since this is such a large restructuring/change, there may be additional issues that come up as we test things, but I think this is in a good state to merge and follow up on subsequent problems we encounter.

@marinaaisa
Copy link
Member Author

@swift-ci test

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.

3 participants