fix(modal): add padding to overflow indicator, sets hasScrollingContent default to false #4775
Conversation
ghost
requested review from
|
Deploy preview for the-carbon-components ready! Built with commit 3d2c2c2 https://deploy-preview-4775--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 3d2c2c2 https://deploy-preview-4775--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 3d2c2c2 |
| @@ -372,6 +373,8 @@ export default class Modal extends Component { | |||
| } | |||
| : {}; | |||
|
|
|||
| console.log(hasScrollingContent); | |||
There was a problem hiding this comment.
You meant to remove this, right?
| @@ -168,7 +168,7 @@ export default class Modal extends Component { | |||
| modalLabel: '', | |||
| selectorPrimaryFocus: '[data-modal-primary-focus]', | |||
| focusTrap: true, | |||
| hasScrollingContent: true, | |||
| hasScrollingContent: false, | |||
There was a problem hiding this comment.
This is a breaking change - Any more details wrt why you thought we need this change?
There was a problem hiding this comment.
I think actually having it set to true was the breaking change because existing code would have to be updated because of aria-label now needing to be defined and there is no default value for it specified for this component.
There was a problem hiding this comment.
just for some context the default value was initially decided in #3943 (comment), but since we now have a gradient to indicate scrolling content I guess we will default back to hasScrollingContent = false?
There was a problem hiding this comment.
@emyarod yeah, I think things got confused in the long thread. I think the reason we can't default to true is that it would break existing usage (as they don't specify aria-label). But if they include the prop, aria-label should be set. I don't think this is included in a stable version yet so can revert safely but will defer to you all in case it has been included 👍
|
Deploy preview for carbon-elements failed. Built with commit ddcf33d https://app.netlify.com/sites/carbon-elements/deploys/5deecec75c1e98000842074e |
|
Deploy preview for carbon-components-react ready! Built with commit ddcf33d https://deploy-preview-4775--carbon-components-react.netlify.com |
|
Deploy preview for the-carbon-components ready! Built with commit ddcf33d https://deploy-preview-4775--the-carbon-components.netlify.com |
joshblack
changed the title
Co-Authored-By: Josh Black <josh@josh.black>
|
@emyarod I'll address that! I noticed that the solution doesn't seem to work in Firefox so I'm looking into fixing that as well |
|
@emyarod I fixed the FF issue and ci is passing now if you want to review! |
|
Has this feature been included in a release yet? If not, then I don't think there is a need to communicate this. Let me know if that's not the case 👀 |
|
@abbeyhrt is there any way we could update the content to use padding-bottom instead of margin and have this work? Would make the scrollbar on the right hand side span almost the full height which would be great, let me know if this doesn't work with the gradient though 👍 |
|
@joshblack I believe that's what I had before but it didn't fix the problem in Firefox, I think because of how it interprets |
|
I think we could figure out the padding bit, but also noticed this weird clipping thing:
Did you notice it as well? On the bottom edge, seems like the outline is getting overlapped by the content |
…fix-modal-overflow
This resolves the problem you can see here
by setting the default of
hasScrollingContentto false. Adds padding to modal content's last child so the overflow indicator goes away when the bottom of the content is hit.Changelog
Changed
hasScrollingContentprop to false by default.padding-bottomof modal content to2remRemoves
carbon--from spacing tokens.Testing / Reviewing
Go to the modal content examples and make sure that the overflow indicator shadow goes away when the bottom of the content is reached and that the modal content isn't fading when there is no scrollable content.