-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ContextualSaveBar] Add support for custom saveAction
#10249
Conversation
adcb04e
to
bd34872
Compare
&:active svg, | ||
&.pressed svg { | ||
fill: var(--p-color-icon-critical-strong-active-experimental); | ||
} | ||
&:hover { | ||
// stylelint-disable-next-line -- Button custom properties | ||
--pc-button-icon-color: var( | ||
--p-color-icon-critical-strong-hover-experimental | ||
); | ||
} | ||
|
||
&.disabled svg { | ||
fill: var(--p-color-icon-disabled); | ||
} |
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.
The two declarations were duplicates once the unneeded svg
tag is removed from it, so the custom property is set at the top of the equivalent class' declarations on 606 and 664.
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.
(ignore)
// stylelint-disable-next-line -- Button custom properties | ||
--pc-button-icon-color: var( | ||
--p-color-icon-critical-strong-active-experimental | ||
); |
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.
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.
(ignore)
// stylelint-disable-next-line -- Button custom properties | ||
--pc-button-icon-color: var(--p-color-icon-disabled); |
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.
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.
(ignore)
@@ -40,7 +40,7 @@ export interface ContextualSaveBarProps { | |||
/** Accepts a string of content that will be rendered to the left of the actions */ | |||
message?: string; | |||
/** Save or commit contextual save bar action with text defaulting to 'Save' */ | |||
saveAction?: ContextualSaveBarAction; | |||
saveAction?: ContextualSaveBarAction | React.JSX.Element; |
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.
(ReactNode is too wide of a net so this is more specific, as we only want to accept JSX elements not strings etc)
060c785
to
3b3f0d0
Compare
- [Migrating from v11 to v12](#migrating-from-v11-to-v12) | ||
- [Table of Contents](#table-of-contents) | ||
- [Quick migration guide](#quick-migration-guide) |
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 happens automatically no matter what extension I disable 😤
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.
(ignore)
svg { | ||
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY | ||
fill: var(--pc-button-icon-color); | ||
} |
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.
Instead of this being declared over and over again, it's now declared once and the custom property is set in the class and interaction state declarations instead.
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.
(ignore)
22b17e2
to
3bbebd5
Compare
9cc8232
to
48ca178
Compare
@chloerice Is this still being worked on? Or is this work now a part of the overlay patterns project? |
Yes, I'll jump back into this Monday! I'd held off on it because I was updating the way we handle SVG fill in Button (nerd-sniped 😅) and wanted to split that out. Sam did this in the Button refactor, so I can just clean this branch up to only the CSB changes 🚀 |
@chloerice Awesome, thanks so much Chloe! Also just wanted to let you know I created an accompanying issue for this PR since I got a request to remove any PRs from our project board. Feel free to edit the issue if needed! |
3bbebd5
to
d0c0b7c
Compare
d0c0b7c
to
0821a3c
Compare
0821a3c
to
5aeb04e
Compare
5aeb04e
to
f690f9a
Compare
is it possible to hold off on this until we redesign the CSB? it might be harder for us to move/override the CSB save if we don't know for sure that it is a button. We're also introducing a new button style so not sure how it would work for the connected disclosure. Is this needed somewhere in the admin right now? |
Yes, this totally makes sense to hold off on 💯 Moved to Icebox and noted in description!
@sophschneider No, not that I'm aware of 👀 It was just a regression of support when we removed the |
f690f9a
to
6a517e9
Compare
Note
This PR should not be shipped until the context bar redesign ships and we know whether or not this is still necessary to support.
WHY are these changes introduced?
Fixes https://github.com/Shopify/polaris-internal/issues/1376
ContextualSaveBar
doesn't accept a ReactNode forsaveAction
right now, so the improved API for split buttons shipped in #10182 can't be implemented in it.WHAT is this pull request doing?
This PR:
saveAction
propButton
color properties to invert theContextualSaveBar
saveAction
Page
example to a realistic admin use case (the split button)How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes