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
[Frame] add offset prop and pass it through theme provider #2887
Conversation
162643d
to
77e7301
Compare
77e7301
to
cffb91c
Compare
cffb91c
to
326e5f8
Compare
c8cc9d6
to
2e91c5c
Compare
efe1180
to
fee7518
Compare
fee7518
to
58ffda9
Compare
6cb788c
to
88e7736
Compare
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.
Thanks @alex-page! Addressed here: 1002221 |
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.
I'm happy with this. Would love another set of eyes though.
609bf06
to
d236cd2
Compare
Besides loading bar, any issues with other fixed position elements like |
Loading bar issue has been addressed here: 1002221 Things like modal and toast aren't dependent on frame. They're rendered in portals and centred on the document and I think they look fine as is. Might look weird with a push right |
What about toast overlaying the global ribbon, does it look bad? |
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 the one comment but tophat looks good to me too.
Can you explain the decision to add this is on the theme config and then passed through context vs the global ribbon is on the frame directly? |
You could just pass this as a |
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.
Add a test, too
I had a partial solution where I was adding the custom property inline. It involved casting the property as |
cc @Shopify/global-nav-web |
) * Remove left: 0 from Frame_Navigation class * Check for existing frame before adding story padding * Add frame offset to theme provider * Appeasing percy * Check for nav and top bar before setting left or top * undo changes to details page * fix readme lint and test * Push loading bar * Add known custom property * Remove unnecessary calc * Update param to number add tests
WHY are these changes introduced?
Fixes https://github.com/Shopify/destinations/issues/664
This adds space for consumers to add either a secondary nav, or a completely new nav altogether
WHAT is this pull request doing?
This adds a
frameOffset
property to the theme provider which generates a custom property that is used to push the frame and its sub components to the right by the pixel value provided.How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Here's the details page with frame offset and global ribbon.
Copy-paste this code in
DetailsPage.tsx
:🎩 checklist
README.md
with documentation changes