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

feat:(react-nav-preview) Adds small size variant #31589

Merged
merged 10 commits into from
Jun 10, 2024

Conversation

mltejera
Copy link
Contributor

@mltejera mltejera commented Jun 5, 2024

Adds small variant to Nav, and a little more pixel pushing for alignments.

Cleaned up a duplicate item in an example.

Added a new example for size.

Ladders to #26649

@mltejera mltejera changed the title feat:(react-nav-preview) feat:(react-nav-preview) Adds small size variant Jun 5, 2024
Copy link

codesandbox-ci bot commented Jun 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 5, 2024

📊 Bundle size report

✅ No changes found

@mltejera mltejera marked this pull request as ready for review June 5, 2024 21:54
@mltejera mltejera requested a review from a team as a code owner June 5, 2024 21:54
Copy link

codesandbox-ci bot commented Jun 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

LGTM! Just left one nit and one question

@lekoulekou
Copy link

Hi hi! Just catching some minor spacing things that seem different between the PR and Figma. Things are looking great overall!! Thanks for being so detail oriented.

Looks like the backplate is currently touching the right of the french fry, but it would be nice to have some space. It should be 4 pixels to the left of the french fry and 2 pixels to the right before the backplate starts.
image

For the NavCategoryItem, I decided to change the right padding a bit smaller so the space between the chevron and scrollbar seems less wonkily large. It was previously 10px (Horizontal/MNudge) but is now 8px (Horizontal/S). None of the other top/left/bottom padding got changed. The text box was previously set to Fixed and is now Fill--not sure if this impacts the code, but wanted to include that just in case it's relevant.
image

Chevron things:

  • The chevron is looking smaller than it should be, can you check to see if it's 20px?
    image
  • Also noticing the chevron doesn't seem vertically aligned in the node height for both the medium and small size examples. In the example for Nav Drawer Default, the chevron alignment seems fine on the first parent node (Job Postings), but then seems to get worse on the other two (Retirement, Career Development). Not sure why that is but might be worth checking out.
    image
    image

Spacing between things in the nav:
Sorry!! I forgot to include in the spec that we're having a 2px space between all nav items, including things like divider. This helps us align with some other Fluent 2 components like Tree and Menu that also have this "space-between" pattern. Here's screenshots of the dev notes for that container and then how it's showing up in the Figma example if it's helpful.
image
image

@mltejera mltejera force-pushed the small-size-variant branch 2 times, most recently from 654aab0 to 29490e5 Compare June 6, 2024 18:42
@lekoulekou
Copy link

Heya Mason!~ This is looking so good. I found some more tiny pixels to push around and organize. All tiny things, we're so close!!

  • Section header: It looks like with all the padding, this space is coming to 40px tall, but it should be 32px.
    image
  • This is probably just 1 pixel, maybe 2. But if you look at the Job Postings category, the two child items underneath look like they're indented a bit. They should be left aligned with Job Postings.
    image
  • The space on the right side of the backplate should be 4px. I think it's coming up as 10px right now when I inspected. Here's Preview and Figma side by side:
    image
  • Hamburger: Can the hamburger move up to sit in the middle of the node height? The 40px is great, but the hamburger feels low. Maybe split the 10px top padding and share with the bottom?
    image
    Thank you!

@mltejera mltejera merged commit 3f43fc3 into microsoft:master Jun 10, 2024
18 checks passed
mainframev pushed a commit to mainframev/fluentui that referenced this pull request Jun 11, 2024
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Jun 11, 2024
…-and-drawer-compat

* master: (43 commits)
  chore: remove react-alert from monorepo (microsoft#31642)
  docs(react-skeleton): extend Skeleton story with SkeletonItem examples (microsoft#31608)
  feat(react-motion): add support for params (microsoft#31566)
  applying package updates
  fix: show default title action in dialog body for modal dialogs (microsoft#31648)
  chore:(react-nav-preview)Remove redundant NavDrawerHeaderNav component. (microsoft#31646)
  Update Accordion Size story to allow collapsing (microsoft#31624)
  fix(react-accordion): deprecate navigation prop (microsoft#31587)
  fix: Drawer story accessibility fixes and docs update (microsoft#31570)
  feat:(react-nav-preview) Adds small size variant (microsoft#31589)
  feat: update divider to use element internals (microsoft#31627)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /3rd batch (microsoft#31601)
  chore:(docs) Adding Jest testing document (microsoft#31554)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /2nd batch (microsoft#31600)
  build(deps): bump tar from 6.1.11 to 6.2.1 (microsoft#31633)
  applying package updates
  fix: allow updating of CSS properties when they are already defined (microsoft#31629)
  fix: corrects the border-color for switch when in the checked state on rest (microsoft#31628)
  chore: update Switch to leverage ElementInternals via Checkbox (microsoft#31613)
  chore: update temporarily codeowners for split-in-two migrated packages to maintain proper PR review assignemnt for outdated branches (microsoft#31616)
  ...
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Jun 11, 2024
* master: (43 commits)
  chore: remove react-alert from monorepo (microsoft#31642)
  docs(react-skeleton): extend Skeleton story with SkeletonItem examples (microsoft#31608)
  feat(react-motion): add support for params (microsoft#31566)
  applying package updates
  fix: show default title action in dialog body for modal dialogs (microsoft#31648)
  chore:(react-nav-preview)Remove redundant NavDrawerHeaderNav component. (microsoft#31646)
  Update Accordion Size story to allow collapsing (microsoft#31624)
  fix(react-accordion): deprecate navigation prop (microsoft#31587)
  fix: Drawer story accessibility fixes and docs update (microsoft#31570)
  feat:(react-nav-preview) Adds small size variant (microsoft#31589)
  feat: update divider to use element internals (microsoft#31627)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /3rd batch (microsoft#31601)
  chore:(docs) Adding Jest testing document (microsoft#31554)
  chore(react-components): split react libraries in two (/library and /stories) - teams-prg /2nd batch (microsoft#31600)
  build(deps): bump tar from 6.1.11 to 6.2.1 (microsoft#31633)
  applying package updates
  fix: allow updating of CSS properties when they are already defined (microsoft#31629)
  fix: corrects the border-color for switch when in the checked state on rest (microsoft#31628)
  chore: update Switch to leverage ElementInternals via Checkbox (microsoft#31613)
  chore: update temporarily codeowners for split-in-two migrated packages to maintain proper PR review assignemnt for outdated branches (microsoft#31616)
  ...
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request Jun 14, 2024
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.

4 participants