Skip to content
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

Remove redundant modal-open class from modal.js #33705

Closed
wants to merge 3 commits into from

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Apr 20, 2021

As scrollbar.js handles body overflowY, it is useless to use an extra class for the same reason

Based on this PR #33551

@GeoSot GeoSot requested review from a team as code owners April 20, 2021 20:28
alpadev
alpadev previously approved these changes Apr 20, 2021
@alpadev
Copy link
Contributor

alpadev commented Apr 20, 2021

I guess you can close #33551 then since that is covered by this PR as well.

@GeoSot
Copy link
Member Author

GeoSot commented Apr 20, 2021

I guess you can close #33551 then since that is covered by this PR as well.

Thank you for your quick review. 😄
You are right, I was hoping to push them one by one, getting first the css-review approval.

@GeoSot GeoSot added this to Inbox in v5.0.1 via automation Apr 22, 2021
@GeoSot GeoSot moved this from Inbox to Approved in v5.0.1 Apr 22, 2021
Copy link

@marco-amorim marco-amorim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even tho it's redundant, you can also look at it like it's more information for the person developing the application, so they can easily see what exactly is overriding the default scrolling behavior of the body.

@GeoSot GeoSot force-pushed the gs-modal-remove-redundant-modal_open-class branch from 6f8fe38 to 0d97e8c Compare May 5, 2021 21:06
@GeoSot
Copy link
Member Author

GeoSot commented May 5, 2021

@marco-amorim , as modal, and offcanvas, uses same pieces of code, and both are going to accept different rootElements than body, do you have any proposals which js helper should handle this (scrollbar or backdrop)? (Offcanvas doesn't provide any information till now)

@mdo
Copy link
Member

mdo commented May 6, 2021

I missed that this was removing a class... might be a breaking change if folks have built off that at this point. Thoughts?

@GeoSot GeoSot moved this from Approved to Review in v5.0.1 May 6, 2021
@GeoSot GeoSot force-pushed the gs-modal-remove-redundant-modal_open-class branch from 0d97e8c to 51f9923 Compare May 7, 2021 13:34
@GeoSot
Copy link
Member Author

GeoSot commented May 7, 2021

reverted the 'breaking' change, moving the class toggling functionality to backdrop.js

As an alternative, there is #33551 too that removes only the redundant css.

@GeoSot GeoSot closed this May 10, 2021
@GeoSot GeoSot removed this from Review in v5.0.1 May 10, 2021
@XhmikosR XhmikosR deleted the gs-modal-remove-redundant-modal_open-class branch May 13, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants