Refactored Javascript in a more modern and modular way#865
Open
smallsaucepan wants to merge 7 commits intoMLTSHP:masterfrom
Open
Refactored Javascript in a more modern and modular way#865smallsaucepan wants to merge 7 commits intoMLTSHP:masterfrom
smallsaucepan wants to merge 7 commits intoMLTSHP:masterfrom
Conversation
… Remainder of changes are prettier standardising on double quotes, indenting, and wrapping a few long lines.
… files in anticipation of turning them into real classes. Not entirely necessary, though should help with making sure decoupled from main code. Code only copied verbatim in this commit.
…rk again. Made minimal changes to deliver equivalent behaviour e.g. singleton vs repeated objects per page, load automatically or on demand.
… module. No other code changes.
…g on whether the associated client code is run once, multiple times, or generates concrete objet instances, went with plain module, closures over context variables, or full class definitions. Pulled setCaret and toText out into a common file for reuse.
…e strings. Removed quite a few jQuery .post() and .get() in favour of built in fetch(). Renamed variables to more widely used JS scheme. Switched to async event handlers as a flow on from that in many places.
Member
dieseltravis
left a comment
dieseltravis
left a comment
There was a problem hiding this comment.
As a future enhancement we should add an editorconfig and/or eslint config to keep our styles consistent with this. I can't view the large diffs on my phone but the small ones I went through look great.
| // Per invocation functions that will close over the context dependent | ||
| // variables defined above. | ||
| function clickShowImage(ev) { | ||
| var location = document.location, |
Member
There was a problem hiding this comment.
Can we change all the remaining vars to lets?
Contributor
Author
There was a problem hiding this comment.
Will definitely do this, and make it an eslint rule. Prob left this as wasn't changing the $.get just yet and trying to minimise diffs.
Contributor
Author
|
@bradchoate presuming you're supportive of the direction, how about we create a branch e.g. dev-new-js and merge this PR into that rather than main? That way we can proceed something like:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An attempt to refactor much of mltshp's Javascript in a more modern way, perhaps as a stepping stone to using a few web components here and there. Changes are significant, so broken up into different commits to hopefully help reviewing and track changes across files.
Briefly:
Still working through testing, though so far the new code seems to be equivalent. Seeking feedback on the changes to date.
There will need to be a follow up PR to introduce some sort of build step. As is, files included from main.js aren't "versioned" so could lead to CDN / caching issues. Probably easier in the long run to generate a single bundle to import.