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.

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

fix(modal): modal-open body class lost when switching between modals #1327

Merged
merged 3 commits into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@IlyaSemenov
Contributor

IlyaSemenov commented Nov 10, 2017

This fixes #1325, preventing modal-open class lost on document body when a new modal opens before the previous one's transition is over.

This sets a new custom class modal-closing on body during transition, I am not sure if that's appropriate. It could probably be better to use a flag variable on this.$root.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 10, 2017

Codecov Report

Merging #1327 into dev will decrease coverage by 0.04%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1327      +/-   ##
==========================================
- Coverage   42.69%   42.64%   -0.05%     
==========================================
  Files         130      130              
  Lines        2621     2624       +3     
  Branches      817      818       +1     
==========================================
  Hits         1119     1119              
- Misses       1189     1192       +3     
  Partials      313      313
Impacted Files Coverage Δ
src/components/modal/modal.vue 14.28% <9.09%> (-0.24%) ⬇️

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 94488c6...e0cde28. Read the comment docs.

@tmorehouse tmorehouse self-requested a review Nov 10, 2017

@tmorehouse tmorehouse changed the title from modal: fix modal-open body class lost on modal switch to fix(modal): modal-open body class lost when switching between modals Nov 10, 2017

Switch to name-spaced custom class
To not interfere with any future Bootstrap V4 classes
@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 10, 2017

We'll need to test this out to ensure that the body padding and fixed/static/sticky element padding/margins are not doubled up when the new modal is opened.

It might be better to trigger a listen (this.$root.$once()) for the event bv::modal::hidden, which is fired after a modal has completely hidden, before opening the new modal, which will ensure all adjustments to navbar padding/margins and body padding have been returned to normal. We use a similar technique in b-carousel to wait for a transition to complete before jumping to another slide.

Basically:

If body already has class `modal-open`:
  postpone opening of new modal via this.$root.$once(`bv::modal::hidden`, this.show)
Else:
  open as normal

I'll make a few tweaks to your PR later today.

@IlyaSemenov

This comment has been minimized.

Contributor

IlyaSemenov commented Nov 10, 2017

Agreed, this makes sense.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 10, 2017

Just did a quick test using the following code:

<div>
  <b-btn v-b-modal.modal1>Launch demo modal</b-btn>
  <b-modal id="modal1" title=Modal #1">
    <p class="my-4">Hello from modal!</p>
    <b-btn v-b-modal.modal2>Open second modal</b-btn>
  </b-modal>
  <b-modal id="modal2" title="Modal #2">
    <p class="my-4">Hello from modal #2!</p>
  </b-modal>
</div>

And it appears to work well. Note that there is a small delay of the new modal open as it waits for the first modal transition to end.

Could you test it out to make sure it works in your situation?

@tmorehouse tmorehouse merged commit 99e146f into bootstrap-vue:dev Nov 10, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse removed their request for review Nov 10, 2017

@@ -438,21 +448,17 @@
onBeforeLeave() {
this.is_transitioning = true;
this.setResizeEvent(false);
addClass(document.body, 'b-modal-closing');
},
},

This comment has been minimized.

@IlyaSemenov

IlyaSemenov Nov 12, 2017

Contributor

Extra space character sneaked here by mistake.

This comment has been minimized.

@tmorehouse

tmorehouse Nov 12, 2017

Member

Fixed with commit d5a0487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment