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 😄

* 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 <[email protected]> Co-authored-by: Martijn Cuppens <[email protected]> Co-authored-by: Mark Otto <[email protected]>

…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

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

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

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

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 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

This was referenced Jul 27, 2024

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/AspNetCore#34

Open

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/ElastiQuill#61

Open

[Snyk] Security upgrade bootstrap from 4.3.1 to 5.0.0 Omrisnyk/AspNetCore#35

Open

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

Loading

XhmikosR commented May 14, 2020

muhamadamin1992 commented May 14, 2020

muhamadamin1992 commented Jun 7, 2020 •

edited by XhmikosR

Loading

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

Loading

rohit2sharma95 Dec 12, 2020

muhamadamin1992 Dec 12, 2020 •

edited

Loading

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 Loading

XhmikosR commented May 14, 2020

muhamadamin1992 commented May 14, 2020

muhamadamin1992 commented Jun 7, 2020 • edited by XhmikosR Loading

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 Loading

Choose a reason for hiding this comment

rohit2sharma95 Dec 12, 2020

Choose a reason for hiding this comment

muhamadamin1992 Dec 12, 2020 • edited Loading

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

Loading

muhamadamin1992 commented Jun 7, 2020 •

edited by XhmikosR

Loading

muhamadamin1992 Nov 21, 2020 •

edited by rohit2sharma95

Loading

muhamadamin1992 Dec 12, 2020 •

edited

Loading