Skip to content
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

Added animation when modal backdrop is static #29516

Merged
merged 19 commits into from
Oct 25, 2019

Conversation

anjoshigor
Copy link
Contributor

@anjoshigor anjoshigor commented Oct 12, 2019

The problem

When the modal backdrop is static, it should not hide when user clicks outside it or press escape key. But the current behavior wasn't in agreement with this requisite.

more info on issue #26704

The solution

  • When the event KEYDOWN_DISMISS is fired, the static property is verified and another event HIDE_PREVENTED is fired.
  • The event, if not prevented, is handled and a 1.2 scale transition is performed.
  • If anyone wants to ovewrite this behavior, he/she should listen to the new HIDE_PREVENTED event and handle it accordingly.

Evidence

backdrop

Any doubts and suggestion, please contact! 🤘

P.S: I'm planning to update doc in another PR.

Preview: https://deploy-preview-29516--twbs-bootstrap.netlify.com/docs/4.3/components/modal/#static-backdrop

@anjoshigor anjoshigor requested review from a team as code owners October 12, 2019 02:07
@MartijnCuppens
Copy link
Member

Hi @anjoshigor,

Thanks for this contribution. I think we better add an example of this behaviour to our docs, just to make it clearer.

I would also make the classname less attached to the appearance. Something like "highlighted" or "focused" would be more appropriate, although those names aren't perfect yet. Open for suggestions here.

@XhmikosR
Copy link
Member

We definitely need the docs updated in this PR.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anjoshigor for your work 👍

Just one comment to fix, and yes we need the docs in this PR 😄

js/src/modal.js Outdated Show resolved Hide resolved
@rohit2sharma95
Copy link
Collaborator

@MartijnCuppens What about if the class name would be static or modal-static. Because the class is only added when the modal is static. I am just getting inspiration from the class modal-open that is added in the body, when the modal is open.

@MartijnCuppens
Copy link
Member

.modal-static would be better, thanks for the suggestion @rohit2sharma95!

@florianlacreuse
Copy link
Contributor

Thanks @anjoshigor, that was a feature asked by many people (including me)!

From what I see through the gif, clicking inside the modal will trigger the hide prevented animation? It seems weird to me at first sight. I would expect to have the animation only if I click in the modal backdrop area.

@anjoshigor
Copy link
Contributor Author

@florianlacreuse actually, the gif is kind of misleading. When the cursor is inside the modal, I press escape and that's why the animation is triggered 😅

@MartijnCuppens @rohit2sharma95 I'll use modal-static as classname 👍

@XhmikosR @Johann-S I'll add an explanation to the docs, thanks!

@florianlacreuse
Copy link
Contributor

@anjoshigor Oh ok, good then! Thanks for your work.

@XhmikosR
Copy link
Member

@Johann-S shouldn't we backport this to v4-dev?

@Johann-S
Copy link
Member

Yep I think we can do that 👍

@XhmikosR
Copy link
Member

It won't apply clean probably, and we need to change any Hugo specific code to Jekyll, but it's doable. Feel free to approve and we tackle the backport in another PR.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the JS comments? The code is pretty clear here 👍
And it'll be good for your PR

scss/_modal.scss Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

@anjoshigor can you take care of the above comments please?

@anjoshigor
Copy link
Contributor Author

@XhmikosR just pushed some changes. You can take a look to see if it fulfils all requirements

@XhmikosR
Copy link
Member

/CC @MartijnCuppens @mdo

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems cool!

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

Successfully merging this pull request may close these issues.

8 participants