Modal is wider than screen-width
Closed this issue · 15 comments
Description of issue:
- Model extends past width of screen. Cannot scroll to see rest of content
Component in question (if applicable):
- Modal
Hey @siberry, I'm not seeing this in our modal story but that is possibly a result of our modal story being too bare-bones. Can you link to an instance where this is happening for you in a QA environment? Thanks!
I used the modal in this open pr: NYPL/discovery-front-end#1510
These lines fixed the issue:
styling modal
styling on .no-scroll coming from Design System
This is not deployed yet. I can share a screenshot of the issue on Slack. If you determine we should prevent this from happening with the component out-of-the-box, this could be a good issue for me to pick up :)
@EdwinGuzman The CSS you've added for the .no-scroll class should definitely come out of the box for sure.
For the box-sizing you've added for the modal, is that in addition to the .nypl-ds wrapper on the application? Because that seems... odd
I did not add a box-sizing rule to the modal, unless if that question is meant for @siberry ?
You can see an example of the issue on QA LCA - https://qa-librarycard.nypl.org/library-card/new (I did not update this with the fix and you'll see the page's content shift to the left after filling out the address fields and clicking on next).
I did not add a
box-sizingrule to the modal, unless if that question is meant for @siberry ?
Whoops, that was! Sorry for the confusion.
You can see an example of the issue on QA LCA - https://qa-librarycard.nypl.org/library-card/new (I did not update this with the fix and you'll see the page's content shift to the left after filling out the address fields and clicking on next).
And just to clarify, the position: inherit fixed this behavior?
Yes, exactly.
@siberry, looks like there's a simple fix. You want to take on adding it to the library and running the release?
Finally added the .nypl-ds class to the modal and that did indeed fix the issue. Thanks!
@siberry, you'll probably want to add .nypl-ds to the wrapper for your application, not just the modal, or you'll find visual issues across all the components.
I kind of want to hold off on that because of how many sources we are pulling in styles from. I think we should raise this as an issue for making SCC the default catalog. An audit of our styles is a little overdue. We just have to check everything for regressions.
@siberry, looks like there's a simple fix. You want to take on adding it to the library and running the release?
Oh missed this comment. Sure. Will give it a try 👍
This actually does not appear to be an issue for at least the example in SCC. Since applying the nypl-ds class both position: fixed or inherit work for .no-scroll. @EdwinGuzman are you sure this change is necessary?
I did however add overflow: hidden; to prevent the vertical scroll from displaying for the content behind the modal.
I did however add
overflow: hidden;to prevent the vertical scroll from displaying for the content behind the modal.
This feels like a good addition to the library directly.
Just tested the latest version and adding
.no-scroll {
position: inherit !important;
}
is no longer necessary in the LCA.
The overflow: hidden is a good addition. I recently read how different browsers, however, even when hiding the scrollbar still show something. So that sounds like a good cross-browser testing change.
Re: whether or not to use fixed for the position of .no-scroll, Ellen found a bug in the SCC mobile view on her device. In the version she was looking at, I had overwritten position: fixed. Now, I am removing that change. Just noting here that we should probably keep this styling. Also, maybe this issue can be closed, @EdwinGuzman?
