modal: don't add margin & padding when sticky is not full width #30621

muhamadamin1992 · 2020-04-20T18:24:42Z

XhmikosR · 2020-05-14T11:43:20Z

@muhamadamin1992 Thanks for the patch 🙂 Can you also add a test please?

muhamadamin1992 · 2020-05-14T15:45:23Z

@muhamadamin1992 Thanks for the patch Can you also add a test please?

I can try)

muhamadamin1992 · 2020-06-07T11:52:42Z

@muhamadamin1992 Thanks for the patch 🙂 Can you also add a test please?
The project already has a test that may conflict with my code. What should I do? https://github.com/twbs/bootstrap/blob/main/js/tests/unit/modal.spec.js#L114

muhamadamin1992 · 2020-07-25T15:38:26Z

@muhamadamin1992 Thanks for the patch 🙂 Can you also add a test please?

I was add test can you review please?)

mdo · 2020-09-16T05:33:59Z

@twbs/js-review any chance you can check this out? Wondering if this is ready for alpha 2 or 3.

XhmikosR · 2020-11-03T07:18:29Z

@Johann-S friendly ping for a review :)

js/tests/unit/modal.spec.js

XhmikosR · 2020-11-20T18:49:57Z

js/src/modal.js

@@ -471,6 +471,10 @@ class Modal {
      // Adjust fixed content padding
      SelectorEngine.find(SELECTOR_FIXED_CONTENT)
        .forEach(element => {
+          if (window.innerWidth > element.clientWidth + this._scrollbarWidth) {


Maybe we should move this to a private function?

maybe better move to private function like this?

function setModalSpaces({ element, attributeName, cssPropName }) { if (window.innerWidth > element.clientWidth + this._scrollbarWidth) { return } const actualValue = element.style[attributeName] const calculatedPadding = window.getComputedStyle(element)[cssPropName] Manipulator.setDataAttribute(element, `bs-${cssPropName}`, actualValue) element.style[attributeName] = `${Number.parseFloat(calculatedPadding) + this._scrollbarWidth}px` }

and call this here like

SelectorEngine.find(SELECTOR_FIXED_CONTENT) .forEach(element => { setModalSpaces({ element, attributeName: 'paddingRight', cssPropName: 'padding-right', }); }); SelectorEngine.find(SELECTOR_STICKY_CONTENT) .forEach(element => { setModalSpaces({ element, attributeName: 'marginRight', cssPropName: 'margin-right', }); });

Can you make the update, please? @muhamadamin1992

Yes) Sorry I wrote comment above 3 weeks ago, but forgot submit(

setModalSpaces({ element, attributeName, cssPropName }) { if (window.innerWidth > element.clientWidth + this._scrollbarWidth) { return } const actualPadding = element.style[attributeName] const calculatedPadding = window.getComputedStyle(element)[cssPropName] Manipulator.setDataAttribute(element, cssPropName, actualPadding) element.style[attributeName] = `${Number.parseFloat(calculatedPadding) + this._scrollbarWidth}px` }

rohit2sharma95 · 2020-12-07T19:55:50Z

js/tests/unit/modal.spec.js

+      modalEl.addEventListener('hidden.bs.modal', () => {
+        const currentMargin = Number.parseInt(window.getComputedStyle(stickyTopEl).marginRight, 10)
+
+        expect(stickyTopEl.getAttribute('data-margin-right')).toEqual(null, 'data-margin-right should be cleared after closing')
+        expect(currentMargin).toEqual(originalMargin, 'sticky element margin should be reset after closing')
+        done()
+      })


If the margin is not adjusted while showing modal then IMO, you should not check after the modal is hidden. It seems unnecessary 🤔

rohit2sharma95 · 2020-12-07T20:26:46Z

js/tests/unit/modal.spec.js

@@ -142,6 +142,30 @@ describe('Modal', () => {
      modal.toggle()
    })

+    it('should not adjust the inline margin and padding of sticky and fixed elements when element do not have full width', done => {
+      fixtureEl.innerHTML = [
+        '<div class="sticky-top" style="margin-right: 0px; padding-right: 0px; width: calc(100vw - 50%)"></div>',


While debugging I found that the test was failed because the clientWidth of the element was greater than window.innerWidth. So I changed it to width: calc(100vw - 50%). I am not sure about it, seems like a hack to me 😄

- Update unit test as well

* Add a new offcanvas component * offcanvas.js: switch to string constants and `event.key` * Remove unneeded code * Sass optimizations * Fixes Make sure the element is hidden and not offscreen when inactive fix close icon negative margins Add content in right & bottom examples Re-fix bottom offcanvas height not to cover all viewport * Wording tweaks * update tests and offcanvas class * separate scrollbar functionality and use it in offcanvas * Update .bundlewatch.config.json * fix focus * update btn-close / fix focus on close * add aria-modal and role return focus on trigger when offcanvas is closed change body scrolling timings * move common code to reusable functions * add aria-labelledby * Replace lorem ipsum text * fix focus when offcanvas is closed * updates * revert modal, add tests for scrollbar * show backdrop by default * Update offcanvas.md * Update offcanvas CSS to better match modals - Add background-clip for borders - Move from outline to border (less clever, more consistent) - Add scss-docs in vars * Revamp offcanvas docs - Add static example to show and explain the components - Split live examples and rename them - Simplify example content - Expand docs notes elsewhere - Add sass docs * Add .offcanvas-title instead of .modal-title * Rename offcanvas example to offcanvas-navbar to reflect it's purpose * labelledby references title and not header * Add default shadow to offcanvas * enable offcanvas-body to fill all the remaining wrapper area * Be more descriptive, on Accessibility area * remove redundant classes * ensure in case of an already open offcanvas, not to open another one * bring back backdrop|scroll combinations * bring back toggling class * refactor scrollbar method, plus tests * add check if element is not full-width, according to #30621 * revert all in modal * use documentElement innerWidth * Rename classes to -start and -end Also copyedit some docs wording * omit some things on scrollbar * PASS BrowserStack tests -- IOS devices, Android devices and Browsers on Mac, hide scrollbar by default and appear it, only while scrolling. * Rename '_handleClosing' to '_addEventListeners' * change pipe usage to comma * change Data.getData to Data.get Co-authored-by: XhmikosR <xhmikosr@gmail.com> Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com> Co-authored-by: Mark Otto <markdotto@gmail.com>

…ports #30621)

modal: don't add margin & padding when sticky is not full width

muhamadamin1992 requested a review from a team as a code owner April 20, 2020 18:24

XhmikosR added the js label May 14, 2020

mdo changed the base branch from master to main June 16, 2020 19:58

Seth10001 mentioned this pull request Jul 15, 2020

Modal script adds margin-right and padding-right for .sticky-top #27071

Closed

XhmikosR added the needs tests label Jul 15, 2020

XhmikosR requested a review from Johann-S July 25, 2020 04:41

XhmikosR added v4 v5 labels Jul 25, 2020

muhamadamin1992 force-pushed the modal-with-sticky branch from a38408f to f1dc149 Compare July 25, 2020 15:22

mdo added this to Inbox in v5.0.0-alpha3 via automation Sep 16, 2020

XhmikosR added backport-to-v4 and removed needs tests labels Nov 9, 2020

XhmikosR changed the title ~~don't add margin and padding when sticky not full width~~ modal: don't add margin and padding when sticky is not full width Nov 9, 2020

mdo added this to Inbox in v5.0.0-beta1 via automation Nov 11, 2020

mdo removed this from Inbox in v5.0.0-alpha3 Nov 11, 2020

rohit2sharma95 reviewed Nov 20, 2020

View reviewed changes

js/tests/unit/modal.spec.js Outdated Show resolved Hide resolved

XhmikosR reviewed Nov 20, 2020

View reviewed changes

XhmikosR removed this from Inbox in v5.0.0-beta1 Nov 25, 2020

XhmikosR added this to Inbox in v5.0.0-beta2 via automation Nov 25, 2020

XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Nov 25, 2020

rohit2sharma95 force-pushed the modal-with-sticky branch from 9fcccad to 0253cc5 Compare December 7, 2020 19:48

rohit2sharma95 reviewed Dec 7, 2020

View reviewed changes

rohit2sharma95 requested a review from XhmikosR December 7, 2020 20:27

Check if element is shorter than window

f4fef6c

- Update unit test as well

rohit2sharma95 force-pushed the modal-with-sticky branch from 5ddb40a to f4fef6c Compare February 19, 2021 17:52

rohit2sharma95 removed the has conflicts label Feb 19, 2021

rohit2sharma95 approved these changes Feb 22, 2021

View reviewed changes

v5.0.0-beta3 automation moved this from Inbox to Approved Feb 22, 2021

rohit2sharma95 and others added 2 commits February 23, 2021 00:30

Merge branch 'main' into modal-with-sticky

a0b7db4

Merge branch 'main' into modal-with-sticky

af11a56

XhmikosR merged commit 056216a into twbs:main Feb 23, 2021

v5.0.0-beta3 automation moved this from Approved to Done Feb 23, 2021

XhmikosR added this to Inbox in v4.6.1 via automation Feb 23, 2021

XhmikosR moved this from Inbox to Needs manual backport in v4.6.1 Feb 23, 2021

GeoSot added a commit to GeoSot/bootstrap that referenced this pull request Feb 24, 2021

add check if element is not full-width, according to twbs#30621

8288e9a

XhmikosR pushed a commit to GeoSot/bootstrap that referenced this pull request Mar 2, 2021

add check if element is not full-width, according to twbs#30621

df053ae

GeoSot added a commit that referenced this pull request Oct 11, 2021

modal: don't add margin & padding when sticky is not full width (Back…

05c3f7b

…ports #30621)

GeoSot mentioned this pull request Oct 11, 2021

modal: don't add margin & padding when sticky is not full width #35164

Closed

GeoSot added a commit that referenced this pull request Oct 22, 2021

modal: don't add margin & padding when sticky is not full width (Back…

fc78d52

…ports #30621)

XhmikosR pushed a commit that referenced this pull request Oct 22, 2021

Backport #30621

7c7b2a8

modal: don't add margin & padding when sticky is not full width

XhmikosR removed this from Needs manual backport in v4.6.1 Oct 22, 2021

XhmikosR pushed a commit that referenced this pull request Oct 22, 2021

Backport #30621

28144d8

modal: don't add margin & padding when sticky is not full width

XhmikosR pushed a commit that referenced this pull request Oct 26, 2021

Backport #30621

ffdf96f

modal: don't add margin & padding when sticky is not full width

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modal: don't add margin & padding when sticky is not full width #30621

modal: don't add margin & padding when sticky is not full width #30621

muhamadamin1992 commented Apr 20, 2020 •

edited by XhmikosR

XhmikosR commented May 14, 2020

muhamadamin1992 commented May 14, 2020

muhamadamin1992 commented Jun 7, 2020 •

edited by XhmikosR

muhamadamin1992 commented Jul 25, 2020

mdo commented Sep 16, 2020

XhmikosR commented Nov 3, 2020

XhmikosR Nov 20, 2020

muhamadamin1992 Nov 21, 2020 •

edited by rohit2sharma95

rohit2sharma95 Dec 12, 2020

muhamadamin1992 Dec 12, 2020 •

edited

rohit2sharma95 Dec 7, 2020

rohit2sharma95 Dec 7, 2020

modal: don't add margin & padding when sticky is not full width #30621

modal: don't add margin & padding when sticky is not full width #30621

Conversation

muhamadamin1992 commented Apr 20, 2020 • edited by XhmikosR

XhmikosR commented May 14, 2020

muhamadamin1992 commented May 14, 2020

muhamadamin1992 commented Jun 7, 2020 • edited by XhmikosR

muhamadamin1992 commented Jul 25, 2020

mdo commented Sep 16, 2020

XhmikosR commented Nov 3, 2020

XhmikosR Nov 20, 2020

Choose a reason for hiding this comment

muhamadamin1992 Nov 21, 2020 • edited by rohit2sharma95

Choose a reason for hiding this comment

rohit2sharma95 Dec 12, 2020

Choose a reason for hiding this comment

muhamadamin1992 Dec 12, 2020 • edited

Choose a reason for hiding this comment

rohit2sharma95 Dec 7, 2020

Choose a reason for hiding this comment

rohit2sharma95 Dec 7, 2020

Choose a reason for hiding this comment

muhamadamin1992 commented Apr 20, 2020 •

edited by XhmikosR

muhamadamin1992 commented Jun 7, 2020 •

edited by XhmikosR

muhamadamin1992 Nov 21, 2020 •

edited by rohit2sharma95

muhamadamin1992 Dec 12, 2020 •

edited