search

ruby / TryRuby

This 4th iteration of TryRuby is a website where you can learn the Ruby language.
https://try.ruby-lang.org
MIT License
226 stars 97 forks source link

Replace opal-jquery with opal-browser #104

Closed sunny closed 2 years ago

sunny commented 2 years ago

Hello TryRuby maintainers!

I’ve been trying to update to the latest jQuery & Bootstrap but found that opal-jquery had raised errors with them. Since there are only a few lines related to jQuery, we might as well switch to opal-browser to ease further updates.

Also, this PR refactors JS code a tiny bit and moves Opal loading to inside of application.js.

hsbt commented 2 years ago

Hi, Thanks for your works. It seems nice for the TryRuby. But I didn't have the knowledge for this migration.

@hmdne Can you review this?

hmdne commented 2 years ago

Thank you, @sunny! I wanted to do it myself, as opal-jquery is in the maintenance state, but decided not to do so, for possible breakage reasons (it would be a great idea to construct some tests...).

It all looks mostly fine, but I have some questions. You can correct those issues, or if you don't, when I will sit to a final review of your patchset, I will correct them myself.

sunny commented 2 years ago

Thank you for the review! I’ve pushed the fixes as well as opting for Browser's FormData.build_query and History#replace.

hmdne commented 2 years ago

I checked it thoroughly and I see no immediate issues. I wanted to make a small change, but can do it later. @sunny please ensure you enable in the future "allow commit access to collaborators" so we will have easier time collaborating :)

Anyway thank you, that's a great patch!

sunny commented 2 years ago

Absolutely, will do that next time! If you can think of a change I’d be happy to help.

Thank you for maintaining TryRuby \o/