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

Convert playpen.js to plain JS #26037

Merged
merged 1 commit into from
Jun 22, 2015
Merged

Convert playpen.js to plain JS #26037

merged 1 commit into from
Jun 22, 2015

Conversation

nhowell
Copy link
Contributor

@nhowell nhowell commented Jun 5, 2015

Since the "Book" already avoids jQuery in its inline script tags and playpen.js is tiny, I figured I would convert it to plain old JS as well.

Side note: This is a separate issue, but another thing I noticed in my testing is that the "⇱" character doesn't display correctly in Chrome on Windows 7. (Firefox and IE work fine; other browsers not tested)

r? @steveklabnik

Edit: Github didn't like the "script" tag above
Edit 2: Actually, now IE seems to render "⇱" fine for me. Odd.

@Gankra
Copy link
Contributor

Gankra commented Jun 5, 2015

Wow, nice! Obviously jquery gives us some pretty nice browser compat guarantees. What have you tested this on, and what do you expect this to not work on?

@nhowell
Copy link
Contributor Author

nhowell commented Jun 5, 2015

Thanks, Gankro!

I took some tips from http://youmightnotneedjquery.com/. I have personally tested it on Firefox 38, Chrome 43, and IE 11 (also tested it in IE 10 and IE 9 mode via the developer tools) on Win 7.

I expect IE 8 and lower to not work (which would be the same as it is today).

Also, the "mouseenter" and "mouseleave" events may have less support than jQuery, see:
https://developer.mozilla.org/en-US/docs/Web/Events/mouseenter#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/Events/mouseleave#Browser_compatibility

If it's a problem, I think I could re-write it to use the less-friendly, but standardized, (Edit: actually, the mouseenter/leave events are standardized: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-mouseenter) "mouseover" and "mouseout" events instead.

@Gankra
Copy link
Contributor

Gankra commented Jun 5, 2015

Compatibility looks adequate. I'm inclined to r+, but I'm hesitant to do so without checking out a live build. Would you be able to build of the docs with this change and host it somewhere we can poke at to verify nothing's broken? If this isn't easy for you, I don't think it's necessary since this is pretty self-contained.

@nhowell
Copy link
Contributor Author

nhowell commented Jun 5, 2015

Cool, alright.

It's not terribly easy for me, but I think I have my machine at home set up to compile rust and build docs now, so I can give it a whirl this evening. I can host it someplace, no problem.

@killercup
Copy link
Member

Nice!

You don't need to build the entirety of rustc for this. Just copy the rendered docs folder from your install and replace the JS files :)

@nhowell
Copy link
Contributor Author

nhowell commented Jun 5, 2015

@killercup Oh, that's actually how I worked on this and tested it locally. I assumed @granko wants the full build since this PR does touch the build.rs and javascript.rs files as well, though.

But if copying the rendered docs and replacing the JS files is sufficient, it'll save me some time.

@killercup
Copy link
Member

Well, you are just deleting lines there… But let the mighty @gankro decide whether another innocent person should become part of the Elite-That-Can-Build-Rustc™ ;)

@Gankra
Copy link
Contributor

Gankra commented Jun 5, 2015

Yeah feel free to just copy the files.

On Fri, Jun 5, 2015 at 12:36 PM, Nick Howell notifications@github.com
wrote:

@killercup https://github.com/killercup Oh, that's actually how I
worked on this and tested it locally. I assumed @granko
https://github.com/Granko wants the full build since this PR does touch
the build.rs and javascript.rs files as well, though.

But if copying the rendered docs and replacing the JS files is sufficient,
it'll save me some time.


Reply to this email directly or view it on GitHub
#26037 (comment).

@steveklabnik
Copy link
Member

It looks like this PR needs to be rebased and squashed, there's a lot of extra commits in here.

Everyone else has asked the same things I would ask, so r=me after the git situation is fixed up.

@nhowell
Copy link
Contributor Author

nhowell commented Jun 12, 2015

Sorry for the mess. I've finally got this cleaned up and I've put the docs online with the plain JS version of playpen.js as mentioned before.

Examples for your testing:
http://nhowell.github.io/rust/book/guessing-game.html
http://nhowell.github.io/rust/std/option/index.html

@nhowell
Copy link
Contributor Author

nhowell commented Jun 12, 2015

Well, rats. I've noticed that it doesn't work in iOS Safari now. I'll figure out what's wrong and update the PR.

It is still compatible with IE9+.

This removes the jQuery dependency from the "Book" entirely.
@nhowell
Copy link
Contributor Author

nhowell commented Jun 12, 2015

Ok, I've fixed it up to work in iOS Safari. Give it a test (same links as above) and see what you guys think.

I had to use mouseover/mouseout instead of mouseenter/mouseleave due to Safari's lack of support.

@nhowell
Copy link
Contributor Author

nhowell commented Jun 22, 2015

@steveklabnik Just wanted to ping you to see if this PR can be approved. Thanks.

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jun 22, 2015

📌 Commit 95dc32d has been approved by steveklabnik

@steveklabnik
Copy link
Member

Thanks nick!

@bors
Copy link
Contributor

bors commented Jun 22, 2015

⌛ Testing commit 95dc32d with merge 2287b4b...

bors added a commit that referenced this pull request Jun 22, 2015
Since the "Book" already avoids jQuery in its inline script tags and playpen.js is tiny, I figured I would convert it to plain old JS as well.

Side note: This is a separate issue, but another thing I noticed in my testing is that the "⇱" character doesn't display correctly in Chrome on Windows 7. (Firefox and IE work fine; other browsers not tested)

r? @steveklabnik

Edit: Github didn't like the "script" tag above
Edit 2: Actually, now IE seems to render "⇱" fine for me. Odd.
@bors bors merged commit 95dc32d into rust-lang:master Jun 22, 2015
@nhowell nhowell deleted the plain_js_playpen branch June 27, 2015 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants