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

Fix crash if swal2 action buttons classes are applied to elements in html prop #1420

Merged
merged 3 commits into from Feb 20, 2019

Conversation

Projects
None yet
2 participants
@gverni
Copy link
Member

commented Feb 19, 2019

Closes #1419

  • Added a function elementBySelector() to select element(s) using a selector string.
  • Changed elementByClass() to make use of the newly defined elementBySelector()
  • Changed the getters for confirm and cancel button to select the default swal2 buttons (i.e. the one inside the div identified by class .swal2-actions

This will:

  • Make the swal2 code more robust adding an extra parameter for the selection of the action buttons
  • Fix the error reported in #1419, that is caused by the following code that assumes that the confirm and cancel buttons are always sibling:

if (innerParams.reverseButtons) {
domCache.confirmButton.parentNode.insertBefore(domCache.cancelButton, domCache.confirmButton)
} else {
domCache.confirmButton.parentNode.insertBefore(domCache.confirmButton, domCache.cancelButton)
}

gverni added some commits Feb 19, 2019

Changes in getters for action buttons
* Added a function elementBySelector() to select element(s) using a
selector string.
* Changed elementByClass() to make use of the elementBySelector()
* Changed the getters for confirm and cancel button to select buttons
inside the "actions" div

@gverni gverni changed the title Allow swal2 styles to be used in `html` propriety Fix crash if swal2 action buttons classes are applied to elements in html prop Feb 20, 2019

@limonte
Copy link
Member

left a comment

LGTM! Thank you @gverni!

@limonte limonte merged commit a21ef6b into master Feb 20, 2019

5 checks passed

WIP Ready for review
Details
bundlesize dist/sweetalert2.all.min.js: 14.5KB < maxSize 15KB (gzip)(20B larger than master, careful!)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

limonte pushed a commit that referenced this pull request Feb 20, 2019

chore(release): 8.2.2 [skip ci]
## [8.2.2](v8.2.1...v8.2.2) (2019-02-20)

### Bug Fixes

* crash if swal2 action buttons classes are applied to elements in html prop ([#1420](#1420)) ([a21ef6b](a21ef6b))
@limonte

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

🎉 This PR is included in version 8.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@limonte limonte added the released label Feb 20, 2019

@gverni

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

Hi @limonte your test passes even with the bug present. Infact the issue reported stops the swal execution after the swal is built but before is shown. At the time the error appear both the button defined in html tag and the one in the actions divs are present.

image

Is my understanding correct?

@limonte limonte deleted the fix/swal2-styles-in-html branch Feb 20, 2019

@limonte

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Oops, apologies about that change, I'll revert it back.

@limonte

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

done: 0170a08

thanks for noticing that!

@gverni

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

No problem 😄 One thing I noticed while designing that test is that the Swal.isVisible() is quite a misleading name. In fact it's just checking if the swal has been built but not if it's actually visible on screen:

export const isVisible = () => {
return !!dom.getPopup()
}

If you execute that method on the example provided for this issue, it returns true which is wrong:

image

What do you think @limonte? Should we adjust that as well? Or is it considered a breaking change?

@limonte

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

We should, I would consider that as a bug-fix, not as a breaking change. In the screenshot above Swal.isVisible() should return false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.