-
Notifications
You must be signed in to change notification settings - Fork 322
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(ModalBasicLayout): basic layout component for new Modal #2459
feat(ModalBasicLayout): basic layout component for new Modal #2459
Conversation
153d94f
to
23b868f
Compare
to avoid overlapping top actions with the title (it should stop `spacing-medium`px before the actions)
1380247
to
4986654
Compare
packages/core/src/components/ModalNew/layouts/ModalBasicLayout/ModalBasicLayout.tsx
Show resolved
Hide resolved
ref: React.ForwardedRef<HTMLDivElement> | ||
) => { | ||
const [header, content] = React.Children.toArray(children); | ||
const { contentScrolled } = useModal(); |
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.
Should be isContentScrolled
?
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 was wondering about that one, I always get wondering what is a better practice
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.
On base elements it should be as close to native attrs IMO, other than that - if it's not intuitive enough (like disabled) than it should imply the type
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.
not sure I'm following
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.
<Button disabled>
vs <ModalAction isContentScrolled>
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.
got ya
will change then 🙏🏼
packages/core/src/components/ModalNew/layouts/ModalBasicLayout/ModalBasicLayout.tsx
Outdated
Show resolved
Hide resolved
…lled per to CR request
Basic layout
Usage example:
https://monday.monday.com/boards/3532714909/pulses/7360987899