feat(modal): introduce size variants #4657
Conversation
|
Deploy preview for the-carbon-components ready! Built with commit d7d00f7 https://deploy-preview-4657--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit d7d00f7 https://deploy-preview-4657--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit d7d00f7 |
There was a problem hiding this comment.
widths, max-height, and right padding looks good to me at all breakpoints for all variants
I think we can increase the height of the overflow fade a little or position it a little higher maybe, but will defer to design on this
do we also need to implement logic that removes the overflow indicator when the user has scrolled to the bottom of the content?
| .#{$prefix}--modal-header, | ||
| .#{$prefix}--modal-content, | ||
| .#{$prefix}--modal-content__regular-content { | ||
| padding-right: rem(16px); |
There was a problem hiding this comment.
should we use a spacing token here instead?
There was a problem hiding this comment.
Please see: #4657 (comment)
Still a good catch, though, as the code I referred to doesn't use rem() function. Rectified at: 164817e
| } | ||
|
|
||
| .#{$prefix}--modal-content--with-form { | ||
| padding-right: 1rem; |
There was a problem hiding this comment.
should we use a spacing token here instead?
There was a problem hiding this comment.
Seems that the modal content historically uses fixed size: https://github.com/carbon-design-system/carbon/blob/v10.7.0/packages/components/src/components/modal/_modal.scss#L100
@aagonzales Do you think it's token-based instead?
There was a problem hiding this comment.
Margin-right is the special one because sometimes its 20%. I leave it up to dev to decide if its better to use 1rem or spacing-05.
There was a problem hiding this comment.
Thanks @aagonzales for your response - Updated with spacing tokens.
| padding-right: 1rem; | ||
|
|
||
| @include carbon--breakpoint(xlg) { | ||
| padding-right: 1rem; // Override for `.#{$prefix}--modal-content` |
There was a problem hiding this comment.
should we use a spacing token here instead?
There was a problem hiding this comment.
There was a problem hiding this comment.
I was referring to the carbon--spacing tokens, I think we can use $spacing-05 right?
There was a problem hiding this comment.
Looking good!
- There appears to be a max-width being applied at 768px. Large, small and default are all showing at the same size when on my large display. They should still be rendering at different widths on a large display.
- Agreed with Andrew about the overflow-indicator. Let's give it a height of 32px instead of 16.
- Margin-right on modals less the
448pxshould only be 16px instead of 20%
Co-Authored-By: emyarod <[email protected]>
Co-Authored-By: emyarod <[email protected]>
|
Thank you for reviewing @aagonzales - Removed the |
Refs #4607.
Changelog
New
bx--modal-container--xs,bx--modal-container--smandbx--modal-container--lgclasses (enabled viasizeprop of<Modal>/<ComposedModal>).bx--modal-content--with-formclass (enabled viahasFormprop of<Modal>/<ModalBody>), which removes the right margin. Non-form contents in there should usebx--modal-content__regular-contentclass.Changed
Testing / Reviewing
Testing should make sure our modal is not broken.