Skip to content

fix(b-modal): check for wether the body is overflowing #6481

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

fix(b-modal): check for wether the body is overflowing #6481

wants to merge 3 commits into from

Conversation

wparad
Copy link

@wparad wparad commented Mar 5, 2021 •
edited
Loading

Describe the PR

Opening a modal incorrectly always pads the body and the modal even when there is no reason to because the scroll bar isn't needed.

image

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix (fixes a boo-boo in the code) - fix(...), requires a patch version update

Does this PR introduce a breaking change? (check one)

  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (i.e. [...] (fixes #xxx[,#xxx]), where "xxx" is the issue number)
  • It should address only one issue or feature. If adding multiple features or fixing a bug and adding a new feature, break them into separate PRs if at all possible.
  • The title should follow the Conventional Commits naming convention (i.e. fix(alert): not alerting during SSR render, docs(badge): update pill examples, chore(docs): fix typo in README, etc.). This is very important, as the CHANGELOG is generated from these messages, and determines the next version type (patch or minor).

@vercel
Copy link

vercel bot commented Mar 5, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/bootstrap-vue/bootstrap-vue/JJQjoXFq6CD7A6rKTf7hKwRe3Mnv
✅ Preview: https://bootstrap-vue-git-fork-wparad-patch-1-bootstrap-vue.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 5, 2021 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7261b13:

Sandbox Source
BootstrapVue Starter Project Configuration
BootstrapVue Nuxt.js Starter Project Configuration

@codecov
Copy link

codecov bot commented Mar 5, 2021 •
edited
Loading

Codecov Report

Merging #6481 (7261b13) into dev (f8caaec) will decrease coverage by 0.06%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##               dev    #6481      +/-   ##
===========================================
- Coverage   100.00%   99.93%   -0.07%     
===========================================
  Files          299      299              
  Lines        10211    10213       +2     
  Branches      2521     2523       +2     
===========================================
- Hits         10211    10206       -5     
- Misses           0        5       +5     
- Partials         0        2       +2
Flag Coverage Δ
unittests 99.93% <75.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/modal/helpers/modal-manager.js 92.39% <75.00%> (-7.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8caaec...7261b13. Read the comment docs.

@wparad wparad changed the title Fix check for isBodyOverflowing Fix(alert) check for isBodyOverflowing Mar 5, 2021
@wparad wparad changed the title Fix(alert) check for isBodyOverflowing Fix(modal) check for isBodyOverflowing Mar 5, 2021
@wparad wparad changed the title Fix(modal) check for isBodyOverflowing Fix(modal) calculate check for isBodyOverflowing correctly Mar 5, 2021
@Hiws
Copy link
Member

Hiws commented Mar 5, 2021 •
edited
Loading

I believe the current behavior is intended.
To avoid having page shift in the background when opening/closing a modal.

Here's how it currently looks on the docs when opening a modal:

KSZdp1jiGW.mp4

Notice how the added padding makes it so the background doesn't shift.

Compared to the docs with your change applied:

NPK5cxxLIN.mp4

@wparad
Copy link
Author

wparad commented Mar 5, 2021 •
edited
Loading

I see, but then you have the opposite problem if the screen doesn't have a scrollbar, and the content is not as wide as the screen. What's the fix then?

@Hiws
Copy link
Member

Hiws commented Mar 5, 2021

I see, but then you have the opposite problem if the screen doesn't have a scrollbar, and the content is not as wide as the screen. What's the fix then?

I'm not quite sure what you mean.
No padding should be applied if there's no scrollbar on the page.

ux9auwXBIE.mp4

https://codepen.io/Hiws/pen/Pobdwzm

@wparad
Copy link
Author

wparad commented Mar 5, 2021 •
edited
Loading

Maybe we just need to check that it is greater than 1px off, because I'm struggling with this:
image

And there's clearly some else off here, because if the body is wider than the screen, scrolling involved, then you still have the same problem:

Peek 2021-03-05 21-07

https://codepen.io/wparad/pen/abBazyo

@Hiws
Copy link
Member

Hiws commented Mar 5, 2021

I see what you mean in your codepen, but I'm not sure what can be done about it.
If you have a solution feel free to edit your PR accordingly :)

@wparad
Copy link
Author

wparad commented Mar 10, 2021

I've made an update here, it now checks the body element by length instead of by width.

@jacobmllr95 jacobmllr95 changed the title Fix(modal) calculate check for isBodyOverflowing correctly fix(b-modal): check for wether the body is overflowing Apr 21, 2021
@jacobmllr95 jacobmllr95 self-requested a review April 21, 2021 23:50
@jacobmllr95 jacobmllr95 added PR: Patch Requires patch version bump Type: Fix labels Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobmllr95 jacobmllr95 Awaiting requested review from jacobmllr95

Assignees
No one assigned
Labels
PR: Patch Requires patch version bump Type: Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wparad @Hiws @jacobmllr95