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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Page] Adding additionalNavigation prop #2942

Merged
merged 3 commits into from Apr 24, 2020
Merged

Conversation

dfmcphee
Copy link
Contributor

@dfmcphee dfmcphee commented Apr 24, 2020

WHY are these changes introduced?

For a project I am working on currently, we need to make better use of the space between the breadcrumbs and pagination/actions menu. Currently we have some hacks with absolute positioning, but I would rather this be a part of the page component so we can make it more reliable and also re-arrange if needed (probably in the new design language too). My current implementation gets the wrong tab order as well because I need to place the elements inside the page content instead of in the proper place in the header.

WHAT is this pull request doing?

This adds a new additionalNavigation prop to <Page> that accepts a React Node and renders it in the correct space.

This is what it looks like with an avatar in that spot:

Screen Shot 2020-04-24 at 12 21 12 PM

Screen Shot 2020-04-24 at 12 21 21 PM

How to 馃帺

I added an example to our Page with all header elements example in the README

馃枼 Local development instructions
馃棐 General tophatting guidelines
馃搫 Changelog guidelines

馃帺 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @鈥奌YPD, @鈥妋irualves, @鈥妔arahill, or @鈥妑y5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2020

馃煝 This pull request modifies 5 files and might impact 2 other files.

Details:
All files potentially affected (total: 2)
馃搫 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

馃搫 src/components/Page/README.md (total: 0)

Files potentially affected (total: 0)

馃帹 src/components/Page/components/Header/Header.scss (total: 2)

Files potentially affected (total: 2)

馃З src/components/Page/components/Header/Header.tsx (total: 1)

Files potentially affected (total: 1)

馃З src/components/Page/components/Header/tests/Header.test.tsx (total: 0)

Files potentially affected (total: 0)

Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Code and tab order look good. I'm wondering in the future if we can be a little bit more clever about the positioning of the rollup activator so it's not positioned absolutely and can wrap if need be.

image

As Alex mentioned in slack this does look a little bit tight. If you could add spacing consistent with mobile it would be perfect

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Code looks good. Noticed a minor spacing issue on desktop while tophatting. @kyledurand has more detail above!

@dfmcphee
Copy link
Contributor Author

Thanks for the feedback. I adjusted the spacing a bit. This is what it looks like now:

Screen Shot 2020-04-24 at 1 20 05 PM

Simulator Screen Shot - iPhone 8 - 2020-04-24 at 13 19 50

@dfmcphee
Copy link
Contributor Author

. I'm wondering in the future if we can be a little bit more clever about the positioning of the rollup activator so it's not positioned absolutely and can wrap if need be.

Yeah, that's not a bad idea. I'm not sure if wrapping is best there or what, but would be nice to handle it better.

Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Nice clear separation now 馃憤

@dfmcphee dfmcphee merged commit b608cd1 into master Apr 24, 2020
@dfmcphee dfmcphee deleted the additional-navigation-page branch April 24, 2020 17:46
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.

None yet

3 participants