fix(Modal): fix bug where closing modal removed wrong modal-open string in class#410
fix(Modal): fix bug where closing modal removed wrong modal-open string in class#410eddywashere merged 3 commits intoreactstrap:masterfrom markogresak:master
Conversation
|
|
||
| const classes = document.body.className.replace('modal-open', ''); | ||
| // Use regex to prevent matching `modal-open` as part of a different class, e.g. `my-modal-opened` | ||
| const classes = document.body.className.replace(/(^| )modal-open( |$)/, ''); |
There was a problem hiding this comment.
Best to use word boundaries \b because this will turn my-modal-opened modal-open modal-opened into my-modal-openedmodal-opened which is not wanted. This is because it removes the space from both sides.
/\bmodal-open\b/ would result in my-modal-opened modal-opened. There is double space, but that isn't going to break anything (technically, the current implementation would result in double spaces as well).
There was a problem hiding this comment.
Great catch!
This was adapted from Element.classList polyfill.
I guess I could also replace with ' ' instead of ''.
If you would opt for using \b, I would at least follow the .replace(/\bmodal-open\b/, '') with .replace(/\s{2,}/g, ' '), i.e. replace all occurrences of 2 or more spaces with a single space. Otherwise, it looks weird and could lead to a broken test case if someone would expect single space and would get 2 spaces.
I would also suggest using document.body.classList in the future. Bootstrap 4 supports only IE10+ and classList is already supported in IE10. With classList.remove, there would be no such problem in the first place 😄
…ng in class Example: initial body class: `no-modal-opened` open and close modal, body class: `no-ed modal-open`
|
I pushed a change to fix the problem caught by @TheSharpieOne. I had also left a comment, but GitHub decided to hide it after the changes were pushed. Instead of using word boundary match, I opted in for replacing current regex's match with a single space. If the match is at start/end, the next line uses |
|
thanks @markogresak! |
3 participants
Initially, I wanted to just report the issue, but while writing it, I found out that it was an easy fix so I did it myself.
Here is what I wanted to report:
Issue description
#4.5.0but this issue can be found in the master branchModalSteps to reproduce issue
modal-open, e.g.:my-modal-openedmodal-openclass to the body, the body class property is nowmy-modal-opened modal-openmy-modal-opened, but instead, the class is nowmy-ed modal-openDemo: http://codepen.io/anon/pen/pPWVyP