-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Page] Sync small screen header with native mobile #707
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
Conversation
2c5ff2a
to
9fcd725
Compare
9fcd725
to
80b5a56
Compare
.PrimaryAction { | ||
display: flex; | ||
align-self: stretch; | ||
// align-self: stretch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// align-self: stretch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks for doing this!
Hi all! It looks like this is ready to merge and just needs some Percy review and an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things that need to be fixed with alignment, styles, and rendering
☝️ I'm seeing the same things as @tmlayton when I look at the deployment. When I tested locally with the different variations I wasn't seeing these issues. |
80b5a56
to
5195b61
Compare
5195b61
to
ad444a5
Compare
ad444a5
to
d4c43d2
Compare
d4c43d2
to
8077d57
Compare
@tmlayton @sarahill I've fixed all issues 👍 Be sure to hard refresh and clear your cache when viewing the examples in the latest deployment 😺 Regarding the difference in fill between the example in the issue and this PR: I don't think it would add value to write smelly Sass in order to override the fill of the plain icon button. All plain buttons have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is close!
Left a comment on a couple things.
Hey @sarahill, the current UX for the rollup is that it only rolls up when the actions don't fit. Is part of the desired outcome of this feature request to change that so that actions (other than the primary) always rollup? |
@chloerice Ah I see. That was my original thought but this makes more sense. Show them if we can and roll them up if they don't fit. Thanks for clarifying 👍 and thank you again for doing this 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified the functionality of this with Chloe.
Looks good to me ✅
@sarahill Dug into it further and I think your original thought was right and updated for consistency's sake. Primary button labels can get pretty lengthy in German, don't want to end up in situations like this:
|
8077d57
to
b20287a
Compare
b20287a
to
13e587e
Compare
13e587e
to
f0e9860
Compare
f0e9860
to
42000da
Compare
☝️ nice catch! |
@chloerice this looks like it's ready to go. @tmlayton any objections? |
UNRELEASED.md
Outdated
### Enhancements | ||
|
||
- Moved icons to a separate npm package ([#686](https://github.com/Shopify/polaris-react/pull/686)) | ||
- Updated the `Page.Header` so that actions take up less room on small screens ([#707](https://github.com/Shopify/polaris-react/pull/707)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Updated the `Page.Header` so that actions take up less room on small screens ([#707](https://github.com/Shopify/polaris-react/pull/707)) | |
- Updated the `Page` so that actions take up less room on small screens ([#707](https://github.com/Shopify/polaris-react/pull/707)) |
I would just say Page
because for a consumer that is the only API they know
</DisplayText> | ||
<div className={styles.TitleAndRollup}> | ||
<div className={styles.Title}> | ||
{/* Anonymous divs are here for layout purposes */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a side note, no need to do anything here, but when I see a comment like this I wonder if we can instead write the code in a more meaningful way like creating a component called AnonymousDivForLayout
that just renders a div.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick on the changelog entry, also needs a rebase. Otherwise this looks awesome, so much better! Nice work @chloerice, @HYPD, and @ivymukherjee92
42000da
to
38398f7
Compare
WHY are these changes introduced?
Updates the page component's header to align with the Shopify mobile app's layout, which fixes #706.
WHAT is this pull request doing?
On small screens, the page component's header now rolls up secondary actions and action groups into a plain ellipsis button, which replaces pagination if it exists.
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
git checkout page-mobile-rollup && yarn dev
/playground/Playground.tsx
and navigate to localhost:8080**breadcrumbs
prop to ensure the rollup still aligns right**Click to view collapsed example code
🎩 checklist