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

Remove jQuery and CoffeeScript, add plugin system #160

Merged
merged 59 commits into from
Aug 9, 2016

Conversation

bbatliner
Copy link
Collaborator

@adamschwartz here is a PR to merge my vex2 library back into vex as a major version release.

I had to copy my changes from vex2 back into my fork of vex, in order to make the PR, and lost the commit history - if you're interested in seeing the gritty details of how these changes came to life, you can check out vex2.

For a list of breaking changes, see UPGRADING.md in the root of the project.

In summary:

  1. All of the examples in the original vex documentation are forward compatible with this fork (win!)
  2. vex.dialog got split off and rewritten as a plugin, which I maintain here and use in the docs.
  3. I stuck with ES5/Grunt/Browserify because I didn't feel like vex was complicated enough to warrant switching to newer tech

My biggest question is if my documentation changes are compatible with the HubSpot vex docs at http://github.hubspot.com/vex. Look at the changes I made and let me know!

Thanks!

@bbatliner
Copy link
Collaborator Author

^The version numbers in the commit history are off because this fork was being published on npm and followed semantic versioning. Most of the changes were made in the vex2 repo, and as such don't show up in the commit history of this fork.

@bbatliner
Copy link
Collaborator Author

@geekjuice All systems go for launch 😄 Per @timmfin's suggestion, I think publishing the beta with npm publish --tag next is safest.

@geekjuice
Copy link
Contributor

Let's do this! 💥

@geekjuice geekjuice merged commit 9856652 into HubSpot:master Aug 9, 2016
@geekjuice
Copy link
Contributor

Published as beta in npm and docs have been updated 👍

@bbatliner
Copy link
Collaborator Author

Wow, it looks awesome. Thanks!

Some loose ends that I'm thinking about now:

  • I just pushed a (tiny) update to the docs to update the library size. I hope it's not a big hassle to rebuild the docs, but I know the library size is something I care about when choosing what to use.
  • Did you want me to transfer the vex-dialog repo to HubSpot? Then you can keep the "default" plugin maintained by HubSpot on GitHub, too.
  • I saw some references in the code, and it's still in the docs as a TODO, for "Built in CSS spinner for asynchronous dialogs". Was this ever part of vex and is it something we want to support?

@geekjuice
Copy link
Contributor

geekjuice commented Aug 9, 2016

  1. Not a problem at all 👍 (Should be updated now)
  2. @timmfin Any thoughts on this? Personally, I don't mind that the plugin isn't under the HubSpot namespace.
  3. At this point, any TODO, FIXME, etc. can be taken with a grain of salt. If they seem reasonable and worth adding, then by all means 🚀 , otherwise I think they are deprecated notes now.

@adamschwartz
Copy link
Contributor

Attempting to run the demos on http://github.hubspot.com/vex/api/basic/ and http://github.hubspot.com/vex/api/themes/ is giving me a vex is undefined error. @geekjuice mind taking a look?

@geekjuice
Copy link
Contributor

Ah... seems like I forgot to update a few script tags. Give me one second 😬

@geekjuice
Copy link
Contributor

Updated the links and also fixed up the theme examples. Thanks for catching those 👍

@adamschwartz
Copy link
Contributor

No problem! Thanks for the quick fix 👍

@bbatliner
Copy link
Collaborator Author

I just upgraded our site to the beta version. I haven't found any issues yet! I'll keep an eye out. Thanks for being so active today and making so much progress everyone!

@markstos you should upgrade to the beta, too!

@adamschwartz
Copy link
Contributor

👍

@timmfin
Copy link
Member

timmfin commented Aug 10, 2016

Nice work folks! 🚀 🎉

And I don't have too strong of an opinion, but I'd recommend that we leave vex-dialog under @bbatliner's username for now. If that becomes an issue or confusing, we can adjust.

@markstos
Copy link
Contributor

Is the beta on npmjs.org? There are so many hits for vex on npmjs.org now, it's hard to tel the wheat from the chafe:

https://www.npmjs.com/search?q=vex+

There is: vex, vex-js, vex2, and vex-nojquery, among others. Could some of these be hidden or updated with deprecation notices or links to the main "vex 3" project after the release is final? I realize that the maintainer pool doesn't control /all/ the vex results, but some of these can possibly be changed.

An exciting milestone!

@geekjuice
Copy link
Contributor

I believe those are all different projects. This project is vex-js on npm 👍

@bbatliner
Copy link
Collaborator Author

vex is something else. Unfortunate that this project couldn't use that name on npm.

vex-js, as @geekjuice said, is this project.

vex2 and vex-nojquery (along with vex2-dialog) were published by me (earlier versions of my PR). I've deprecated them all using npm deprecate but I was hesitant to completely unpublish them because I didn't want to break anyone's builds that were still using the unmerged fork.

Once vex 3 has been out for some time, I think it would be safe to unpublish my old versions.

@ericlenio
Copy link

ericlenio commented Aug 11, 2016

Guys I may have found a bug. Method afterOpen used to receive the main content object as the first arg, and vex options in the 2nd arg. In the beta afterOpen is not receiving any args. I guess I was expecting something like this...

diff --git a/src/vex.js b/src/vex.js
index 47a2b0f..521568e 100644
--- a/src/vex.js
+++ b/src/vex.js
@@ -223,7 +223,7 @@ var vex = {

     // Call after open callback
     if (options.afterOpen) {
-      options.afterOpen.call(vexInstance)
+      options.afterOpen.call(vexInstance,contentEl,options)
     }

     // Apply styling to the body

@bbatliner
Copy link
Collaborator Author

Thanks for trying out the beta! The afterOpen callback is changed, as you found out, to be called with the context of the vex instance.

This means you can do the following to access contentEl and options, among other properties:

vex.dialog.alert({
    message: 'Hello, world!',
    afterOpen: function () {
        console.log(this); // the vex instance object
        console.log(this.contentEl); // contentEl is a property of the instance
        console.log(this.options); // so are the options you passed in!
    }
});

@ericlenio
Copy link

OK got it. I had noticed I could do that before I submitted my note, but it seemed a bit too presumptuous about the internals.

@ericlenio
Copy link

Brendan I'm using the 3.0 beta from npmjs.org and it seems that beforeClose is not being called with "this" being set, can you confirm?

@bbatliner
Copy link
Collaborator Author

@ericlenio I think I found the bug you're talking about: https://github.com/bbatliner/vex-dialog/blob/335146007748426330c0b6fc2d9c9ca9e42b2e03/src/vex.dialog.js#L88. The beforeClose callback, when using vex-dialog, isn't called with the right context. I will commit a fix and publish a patch for vex-dialog today. Thanks!

@geekjuice
Copy link
Contributor

@bbatliner Just wanted to check in to see how you feel about taking the current iteration out of beta. Seems like the only blocker was the file structure for cdnjs, but that was resolved a while back.

If things look good in your opinion, I'll rebuild the docs and 🚀 . Thanks for being patient in this process since I wasn't as active I wanted to have been recently 😞

@bbatliner
Copy link
Collaborator Author

Yup, sorry I've been real busy getting back to school. I did some work tonight and we should be good to go 🚀 👍 💥

Having the beta out really helped :) Lots of good feedback from users already.

@bbatliner
Copy link
Collaborator Author

Ah, hold that thought! The quick fix I said I'd commit "today" I never did. Shouldn't be too hard to fix! mb

@bbatliner
Copy link
Collaborator Author

Alrighty, we are go for launch! 🎆

@geekjuice
Copy link
Contributor

👍 I'll create a PR to bump the version out of beta and have that published later today. Thanks again for all of this!

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

Successfully merging this pull request may close these issues.

None yet

6 participants