TobKra96 / music_led_strip_control

Audio visualization for LED strips in real-time with web interface on a raspberry pi.
https://tobkra96.github.io/music_led_strip_control/
MIT License
298 stars 64 forks source link

Refactor dashboard.js #107

Closed weiserhei closed 3 years ago

weiserhei commented 3 years ago

This is my first take on refactoring the Frontend JS Code 🚧

I am about to break the code into modules to introduce OOP and get rid of a lot of duplicated code already existent. There is also some confusing state manipulation and data flow, which can be reduced. I have created a simple Device Class to set things off. Its difficult to identify data-structures and data-flow, I can only introduce them as I go on.

Some parts like Effects Lists should be injected from Python, where we can also generate them from the effect files. This feels a lot better than parsing them out of the HTML with JS. The changes required on the backend files are minor, please let me know if you are comfortable with that.

I am aware that altough this is only a single file, there are a lot of changes. I am trying not to alter any behavior, but its confusing sometimes what is intended and what not. If this is accepted I will continue working on the other server/libs/app/base/static/assets/js/-files. You can follow this WIP or merge anytime, I dont intend to break anything while doing PRs.

Progress:

(Checked does not mean there is nothing left to change, but I am done for the scope of this PR).

Teraskull commented 3 years ago

Is this actually readable code 😁? Nice job, looks clean. A few issues:

  1. For the Device import to work on chromium browsers, you need to add type="module" to this line in dashboard.html:

    <script type="module" src="/static/assets/js/dashboard.js"></script>
  2. Even though the last selected device gets visually restored on reload, the effects are still sent to a wrong device by default. That's where my click() hack was used. Even if you manually click a new device, it still sends to a wrong device.

  3. The all_devices device does not remember its last selected effect correctly.

  4. The click event listener is listening to the whole page, instead of only the effect buttons. (That is the old hack between the // wat (pls refactor) comments.)

I believe 2 and 3 can be fixed by adding

currentDevice = device

to the link.addEventListener() where it is building the device tabs.

weiserhei commented 3 years ago

There is definetly more to improve, looking at the file today I already see some things I have to fix, also considering your points!

1) Its in there, have a look at: https://github.com/TobKra96/music_led_strip_control/pull/107/files#diff-792f18b3e8dc0f687f5214f425670a6881e474de7a3207c52c7c17f373b367e1R305

2) Thanks for pointing out, there is some quirky code still in place. I will tackle that today. I dont think its necessary to build the Device tabs using JS, but we can leave the code if we decide to asynchronously create devices soon. For now I would inject them with python, which also removes the slight delay on pageload when the AJAX request is fired until it gets a response. I plan to do some changes to the create-device-wizard, because for me device creation feels weird on the UX part. Tldr; that could be handled in backend and stripped off anyway😄

Teraskull commented 3 years ago

Ah yes, didn't notice the type="module" change at first, sorry.

The // wat (pls refactor) code can be refactored using an array of effects, and listening directly, instead of searching the whole page and catching errors each time. Something like this:

allEffects.forEach(effect => {
    $("#" + effect).on('click', () => {
        switchEffect(effect);
    });
});
function switchEffect(effect) {
    let newActiveEffect = effect;
    ...

allEffects does not include the Special Effects buttons in the array yet, but you get my point. Something like this:

const allSpecialEffects = $("#dashboard-list-special > div > div").map(function () { return this.id }).toArray();

const allEffects = allSpecialEffects.concat(allNonMusicEffects, allMusicEffects);