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

Add JS Loaders #123

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add JS Loaders #123

wants to merge 4 commits into from

Conversation

lucasvallenet
Copy link
Contributor

@lucasvallenet lucasvallenet commented Jun 2, 2022

Create loaders utils based on Fournier.
Reformat app.js with a js class, and add useful methods (load, init, addCustomEvents, setVars) that are triggered on app mount.

The loaders.js and app.js are W.I.P, improvements are welcome !

@mcaskill mcaskill changed the title JS Loaders Add JS Loaders Jun 2, 2022
Copy link
Member

@mcaskill mcaskill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Regarding the new App class, it's a good convention to have the class and the entry point file be separate:

    A file SHOULD declare new symbols (classes, functions, constants, etc.) and cause no other side effects, or it SHOULD execute logic with side effects, but SHOULD NOT do both.

    The phrase "side effects" means execution of logic not directly related to declaring classes, functions, constants, etc., merely from including the file.
    — PSR-1: Basic Coding Standard, PHP-FIG

  • Following the previous, should window.addEventListener('load', () => this.load()) be defined in the entry point file, after creating the App object?

    const app = new App()
    window.addEventListener('load', () => app.load())
  • Previously, modular was our "Application". Now that a dedicated App class has been introduced, I suggest renaming the property to this.modular, this.manager, or this.moduleManager.

    Or maybe have App extend the modular class?

  • Inline comments (such as "Add custom events", "Set vars", "Window events", "Instanciate app") are somewhat redundant given most of the following lines are self explanatory. If anything, these inline comments should be moved to block comments next to their matching declarations.

@arnvvd
Copy link
Contributor

arnvvd commented Oct 5, 2022

Outcome from meeting on 2022-10-05
Status: On hold until refactoring.

  • We agree with the purpose, but we need some code refactoring. We are waiting for the other merges.

@arnvvd arnvvd added the on hold label Oct 5, 2022
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.

4 participants