Support configuring "root element" for modal #33018

spicalous · 2021-02-08T23:45:22Z

Hi guys,

New contributor here, please let me know if I have missed anything

This PR attempts to resolve #31509 adding support to allow configuring the "root element" for the modal component. This PR only implements configuring this when initialising the modal via Javascript. I haven't dug deep into how this can be done via providing html attributes.

Please let me know if this is the right approach

Thanks!

preview

closes #34309

spicalous · 2021-03-05T13:59:07Z

Hi @XhmikosR is there something I can do to help move this PR along? :)

XhmikosR · 2021-03-05T14:03:53Z

No, sorry, I'm a little busy with Real Life.

Maybe @rohit2sharma95 or @GeoSot can help with review.

For what is worth, I'm definitely in favor of the null tests change.

GeoSot · 2021-03-05T14:55:34Z

@spicalous try to create a working optical example in site/content/docs/5.0/components/modal.md in order to check it here

(You may need to revert it later)

spicalous · 2021-03-05T22:07:45Z

@XhmikosR I didn't mean to rush, thank you for your time and bringing relevant people to the discussion!

@GeoSot I have added an example, please let me know what you think
https://deploy-preview-33018--twbs-bootstrap.netlify.app/docs/5.0/components/modal/#configuring-root-element

site/assets/js/application.js

GeoSot · 2021-03-06T08:50:31Z

I would suggest you to open another Pr with the null changes on tests

I am not sure if CLASS_NAME_OPEN_CUSTOM_ROOT is needed, at least yet. I understand the root element, but I don't know if its ok to restrict the modal in inside a box, But I am not the right guy to answer this

spicalous · 2021-03-06T14:25:14Z

I have created #33288 separately for the tests changes

In the mean time, i'm happy to either decline this or wait until someone can make the decision whether this feature should be added :)

Thanks for your time everyone

GeoSot · 2021-03-09T19:39:56Z

I have created #33288 separately for the tests changes

Spicalous Please remove them from here :)

GeoSot · 2021-04-18T20:15:39Z

@spicalous I apologize for the delay. I had in mind this PR, and tried to make some steps, on backdrop, scrollbar and modal. Do you still want to close it?

spicalous · 2021-04-18T21:22:33Z

Thanks @GeoSot, happy to re-open. Only just saw the mention from #33665 now.

I haven't looked at the changes, but I assume we'll be able to incorporate the solution into this PR? I'll have a look at understanding the changes and will update this branch accordingly

Please let me know if this is what you had in mind or if there's something I should do / can help out with :)

GeoSot · 2021-04-18T21:31:54Z

Scrollbar and backdrop functionality, have been decoupled to be used on offcanvas too,
Backdrop is ready to accept a rootElement, but for scrollbar, I just needed to do it step by step. So I want to try to make the first step on #33665. Please keep an eye on it.

GeoSot · 2021-06-06T21:20:21Z

@spicalous

I think we are in the right moment to continue this MR.
Are ok to continue your proposal?

spicalous · 2021-06-22T20:40:41Z

@GeoSot Had another attempt with the latest master

https://deploy-preview-33018--twbs-bootstrap.netlify.app/docs/5.0/components/modal/#configuring-root-element

Notes

The css doesn't support configurable root elements out of the box... and css is not my strong suite - I think I will need some help to make it better as it feels like a hack at the moment or it will need some testing
.modal-open seems to be an unused class now? So I have removed it

GeoSot · 2021-06-22T20:50:09Z

.modal-open seems to be an unused class now? So I have removed it

bring back .modal-open as we keep it for compatibility reasons

The css doesn't support configurable root elements out of the box... and css is not my strong suite - I think I will need some help to make it better as it feels like a hack at the moment or it will need some testing

I 'll try to help you till there, and we will find a solution

Plus:

Try to replace the _isCustomRoot with a private method hasCustomRoot
try to resolve rootElement inside _getConfig (getElement(config.rootElement))
Scrollbar maybe will need something more, but not too much (leave it for the end 😉 )

spicalous · 2021-06-22T22:42:54Z

js/tests/integration/bundle.js

@@ -1,4 +1,4 @@
-import { Tooltip } from '../../../dist/js/bootstrap.esm.js'
+import { Tooltip } from './../../../dist/js/bootstrap.esm.js'


I had to add this, otherwise the tests was failing for me :/

GeoSot · 2021-06-22T23:34:45Z

Please, keep calm. I am here 😋

Do not mess with backdrop yet. I am trying to help merging #34149

Try to keep your changes to minimum

js/src/modal.js

site/assets/js/application.js

GeoSot · 2021-07-01T11:29:41Z

@spicalous any update on this?

js/src/modal.js

js/src/util/backdrop.js

site/assets/js/application.js

GeoSot · 2021-07-06T09:45:24Z

js/src/util/scrollbar.js

-  constructor() {
-    this._element = document.body
+  constructor(rootElement) {
+    this._element = rootElement || document.body


You miss some things here 😉

plus the proper tests

This MR will help you

I'm not sure how I could add a test in modal (I see in the MR that we are asserting spied methods of ScrollBarHelper. Please let me know if you had something in mind. In the mean time, I have added a test for ScrollBarHelper

@spicalous
I tried to fix it a bit and added the tests and functionality for scrollbar.
I am not very sure about scrollbar modifications, so you better take a second look

GeoSot · 2021-07-06T09:46:36Z

Nice job till here 👍 Getting closer

tip: check the preview. modal.js:l89 misses the proper initialization

- resolve rootElement using getElement() when creating config; - reinstate modal-open class - refactor boolean flag to method

…(body)

GeoSot · 2021-07-12T13:45:10Z

I found out a 'bug'

if you try to put some content inside the empty root element, it will over-float. Till here we are fine.
Although if you try open the modal inside the scrolled down root element, the absolute position, will be out of user eyes 😕

GeoSot · 2021-12-06T10:09:43Z

I am closing this as stale.
@spicalous if you find time, you can reopen it

spicalous requested a review from a team as a code owner February 8, 2021 23:45

XhmikosR added feature js v5 labels Feb 9, 2021

spicalous requested a review from a team as a code owner March 5, 2021 21:56

spicalous force-pushed the modal-root-element-config branch from 550c63c to ea21df6 Compare March 5, 2021 22:01

GeoSot reviewed Mar 6, 2021

View reviewed changes

site/assets/js/application.js Outdated Show resolved Hide resolved

GeoSot mentioned this pull request Apr 17, 2021

Scrollbar.js Sanitizations #33665

Closed

spicalous closed this Apr 17, 2021

spicalous reopened this Apr 19, 2021

GeoSot mentioned this pull request May 12, 2021

Refactor scrollbar.js to be used as a Class #33947

Merged

GeoSot mentioned this pull request Jun 21, 2021

cannot set modal backdrop root element #34309

Open

spicalous force-pushed the modal-root-element-config branch from 41519b7 to 120e344 Compare June 22, 2021 20:34

spicalous commented Jun 22, 2021

View reviewed changes

GeoSot reviewed Jun 22, 2021

View reviewed changes

js/src/modal.js Outdated Show resolved Hide resolved

site/assets/js/application.js Outdated Show resolved Hide resolved

spicalous force-pushed the modal-root-element-config branch from e1eb294 to 0536f6c Compare July 3, 2021 17:44

GeoSot self-assigned this Jul 5, 2021

GeoSot reviewed Jul 5, 2021

View reviewed changes

js/src/modal.js Outdated Show resolved Hide resolved

js/src/util/backdrop.js Outdated Show resolved Hide resolved

site/assets/js/application.js Outdated Show resolved Hide resolved

GeoSot reviewed Jul 6, 2021

View reviewed changes

spicalous and others added 7 commits July 8, 2021 00:21

configurable modal custom root element

03c09f7

PR amendment:

49ea9cb

- resolve rootElement using getElement() when creating config; - reinstate modal-open class - refactor boolean flag to method

remove string from type check as it is already resolved

5cd8aaf

revert: modal-open comment removal

7f71606

use data-bs-root-element; replace custom classes with .modal-open:not…

cc53f48

…(body)

amended scroll bar instantiation with missed root element

f18ce54

Add scrollbar functionality for different rootElements than body

9d1c7b3

GeoSot force-pushed the modal-root-element-config branch from c17aae6 to 9d1c7b3 Compare July 7, 2021 22:17

GeoSot mentioned this pull request Nov 22, 2021

Modals in a div #35311

Closed

2 tasks

GeoSot closed this Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configuring "root element" for modal #33018

Support configuring "root element" for modal #33018

spicalous commented Feb 8, 2021 •

edited by GeoSot

Loading

spicalous commented Mar 5, 2021

XhmikosR commented Mar 5, 2021

GeoSot commented Mar 5, 2021

spicalous commented Mar 5, 2021

GeoSot commented Mar 6, 2021

spicalous commented Mar 6, 2021 •

edited

Loading

GeoSot commented Mar 9, 2021

GeoSot commented Apr 18, 2021

spicalous commented Apr 18, 2021

GeoSot commented Apr 18, 2021

GeoSot commented Jun 6, 2021

spicalous commented Jun 22, 2021

GeoSot commented Jun 22, 2021

spicalous Jun 22, 2021 •

edited

Loading

GeoSot commented Jun 22, 2021

GeoSot commented Jul 1, 2021

GeoSot Jul 6, 2021

spicalous Jul 6, 2021

GeoSot Jul 7, 2021 •

edited

Loading

GeoSot commented Jul 6, 2021 •

edited

Loading

GeoSot commented Jul 12, 2021

GeoSot commented Dec 6, 2021

		@@ -1,4 +1,4 @@
		import { Tooltip } from '../../../dist/js/bootstrap.esm.js'
		import { Tooltip } from './../../../dist/js/bootstrap.esm.js'

Support configuring "root element" for modal #33018

Support configuring "root element" for modal #33018

Conversation

spicalous commented Feb 8, 2021 • edited by GeoSot Loading

spicalous commented Mar 5, 2021

XhmikosR commented Mar 5, 2021

GeoSot commented Mar 5, 2021

spicalous commented Mar 5, 2021

GeoSot commented Mar 6, 2021

spicalous commented Mar 6, 2021 • edited Loading

GeoSot commented Mar 9, 2021

GeoSot commented Apr 18, 2021

spicalous commented Apr 18, 2021

GeoSot commented Apr 18, 2021

GeoSot commented Jun 6, 2021

spicalous commented Jun 22, 2021

GeoSot commented Jun 22, 2021

spicalous Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

GeoSot commented Jun 22, 2021

GeoSot commented Jul 1, 2021

GeoSot Jul 6, 2021

Choose a reason for hiding this comment

spicalous Jul 6, 2021

Choose a reason for hiding this comment

GeoSot Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

GeoSot commented Jul 6, 2021 • edited Loading

GeoSot commented Jul 12, 2021

GeoSot commented Dec 6, 2021

spicalous commented Feb 8, 2021 •

edited by GeoSot

Loading

spicalous commented Mar 6, 2021 •

edited

Loading

spicalous Jun 22, 2021 •

edited

Loading

GeoSot Jul 7, 2021 •

edited

Loading

GeoSot commented Jul 6, 2021 •

edited

Loading