-
Notifications
You must be signed in to change notification settings - Fork 296
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(ModalFooterBase): base footer component to be used by different modal footers #2437
Conversation
aa9edfb
to
8db9939
Compare
width: 100%; | ||
padding: 20px var(--spacing-large); | ||
flex-shrink: 0; | ||
z-index: 1; |
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.
Why?
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.
you mean regarding the z-index?
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.
Yes
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.
probably a leftover, removing
{secondaryButton.text} | ||
</Button> | ||
)} | ||
{renderAction} |
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.
Wondering if it should be children
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.
we're rethinking everything here, but yep it can definitely be children
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.
ok we decided to leave everything here as slots
I want to keep the "children" prop as a safe prop for an option to pass a whole different footer which gets the footer css but can pass a custom one
I'm not sure this use case would ever happen, but you can never know
16dc805
to
c56818d
Compare
8db9939
to
500da09
Compare
…eat/yossi/modalfooterbase-component-to-act-as-a-base-for-advanced-7360692401
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.
💯
.footer { | ||
position: relative; | ||
width: 100%; | ||
padding: 20px var(--spacing-large); |
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.
20 intentional?
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.
ref: React.ForwardedRef<HTMLDivElement> | ||
) => { | ||
return ( | ||
<Flex |
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.
Really nice layout!
153d94f
into
feat/yossi/new-modal-building-blocks-7359960492
ModalFooterBase
Illustration only:
https://monday.monday.com/boards/3532715121/pulses/7360692401