-
Notifications
You must be signed in to change notification settings - Fork 125
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
Modernize ROSE UI Javascript #475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about modern javascript, but I don't have time to maintain this code. If you want to maintain this code and this change makes it easier for you than why not.
Thanks, sure I can help maintain the code ( part time :-) ), @sleviim do you know who currently maintain this code, do you want me to help ? |
Thanks, @yaacov |
I will be happy to help maintain the code, here is some reference for this PR: in the last session of Nitazanim the students where given a task to write a pull request for the ROSE repository, the students had some trouble relate to the code. The ROSE code base uses deprecated dialects: The ROSE code uses refactored logic, students may need more experience to understand: This PR is for making the code more ready for next year Nitzanim task, but IMHO more work need to be done to update ROSE code, for example: |
about #454 |
Sure, please do. |
@cben hi, I am looking for a reviewer, can you review this PR ? |
@sgratch hi, I am looking for a reviewer, can you review this PR ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine. Ideas for improvement:
this looks like a perfect example of a web components use case. Instead of creating a Game
class and having it "mount" on some div
in the DOM, what about having something like this:
class Game extends HTMLElement {
static template = document.createElement('template');
static {
this.template.innerHTML = `
<p>game DOM here</p>
<canvas></canvas>
`;
}
constructor() {
super();
// create private Shadow DOM for the game
this.attachShadow({ mode: 'open' })
// efficiently clone game DOM
.append(Game.template.content.cloneNode(true));
}
connectedCallback() {
// initialize game...
}
}
customElements.define('rose-game', Game);
Then in your page HTML
<rose-game></rose-game>
etc etc
You could also use a web components framework (I recommend Lit) to help with dynamic updates. You don't need a build step to do this, you can load Lit as es modules from a CDN.
I'm available to pair on this if you're interested in learning more
Signed-off-by: yzamir <[email protected]>
@bennypowers thanks for the review ❤️ I have updated the PR according to comment #475 (comment)
Sounds cool ! it will be great idea for the next PR. |
@sleviim @nirs hi, Note: |
var ROSE = (function() { | ||
"use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it was omitted as part of the patch, just to make sure:
Does the UI work as expected and does the game behave as normal?
I haven't tested it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 yes, works for me :-)
Screencast.from.2023-09-06.17-24-09.webm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modules always run in strict mode
As well, all code in class bodies runs in strict mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modules run in strict mode and are always deferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirs wdyt?
…const Signed-off-by: yzamir <[email protected]>
Issue:
Current Javascript user interface uses very old Javascript syntax and depend on jQuery, moving to ES6 (also known as ECMAScript 2015) and removing jQuery from a project can have several benefits:
What are the updates proposed by this PR:
var
withlet
orconst
based on mutability.class
syntax.jQuery
AJAX withFetch
: TheFetch
API is a modern way to make network requests and can replace jQuery's$.ajax
or$.post
.document.querySelector()
ordocument.querySelectorAll()
instead of$()
.addEventListener
instead of jQuery's.click()
, .on()
, etc.