HubSpot / vex

A modern dialog library which is highly configurable and easy to style. #hubspot-open-source
http://github.hubspot.com/vex/docs/welcome
MIT License
6.92k stars 494 forks source link

Adding 'closeAllOnPopState' option #200

Closed bendalton closed 7 years ago

bendalton commented 7 years ago

While this behavior CAN be helpful it is useful to allow users to override the default.

This commit makes this configurable with closeAllOnPopState, updates the documentation, and updates the built versions of vex.

I tested both versions of the configuration and they behave as expected

markstos commented 7 years ago

Note that this builds on PR #177 which was never actually released in ./dist (See #201). Here's what the fix in PR #177 looked like:

diff --git a/src/vex.js b/src/vex.js
index 31b061e..695e4c4 100644
--- a/src/vex.js
+++ b/src/vex.js
@@ -294,7 +294,7 @@ window.addEventListener('keyup', function vexKeyupListener (e) {
   }
 })
 // Close all vexes on history pop state (useful in single page apps)
-window.addEventListener('popstate', vex.closeAll)
+window.addEventListener('popstate', vex.closeAll.bind(vex))

 vex.defaultOptions = {
   content: '',

Here's how the same lines of code are handled here:

 // Close all vexes on history pop state (useful in single page apps)
-window.addEventListener('popstate', vex.closeAll)
+window.addEventListener('popstate', function(){
+  if(vex.defaultOptions.closeAllOnPopState){
+    vex.closeAll.call(vex);
+  }
+})

The new code also solves the binding issue, while further adding an option to disable the behavior of closing all windows on state change.

The "diff" here is so big because the PR tries to avoid the problem from #177 where ./dist never got updated. If this PR is approved now, it would also solve #201.

Going forward, it would be helpful to clearer guidance about wether PRs should update ./dist or not. (It seem like the PRs would be easier to review if they did not update ./dist, but then then it becomes essential that ./dist is updated as part of cutting new releases.

bbatliner commented 7 years ago

My apologies for the delay. This looks good, merging in now. Thank you for catching that.