Add UI elements and logic to let users sort the presets in the preset view #833
Conversation
|
|
||
|
|
||
| function onPresetSortByChanged(ev) { | ||
| localStorage.setItem('preset_list_sort_by', presetSortBy = ev.target.value); |
There was a problem hiding this comment.
there's a lot of odd code format choices in here
There was a problem hiding this comment.
can you provide any specifics on what you find odd about the format?
There was a problem hiding this comment.
This commented line has snuck an inline value assignment in the middle of another line. There's double-blanklines all around. There's usage of syntax features that the rest of the codebase generally doesn't use, eg const and ===
The most important thing code in a PR needs to be, syntax wise, is similar to the other code around it in the same project. Otherwise it becomes an issue down the road for maintenance.
There was a problem hiding this comment.
ok I've gone through and cleaned up the spacing, added comments, and (even though it feels really weird) removed the usages of const and === in favor of the looser let and == syntax.
Let me know if it's still out of line with your JS usage preferences.
| const reverseElem = document.getElementById("preset_list_sort_reverse"); | ||
|
|
||
| if (sortByElem !== presetSortByElement) { | ||
| presetSortByElement?.removeEventListener("change", onPresetSortByChanged); |
There was a problem hiding this comment.
I was running into issues with the actual lifetime of the html elements vs when this code executes. Finally wrote it to defensively check the element each time and add/remove listeners as needed.
There was a problem hiding this comment.
You can just not store it then, and use getRequiredElementById
There was a problem hiding this comment.
but that function will throw if the element does not exist (which it sometimes doesn't when this code runs). Errors about the element not existing is what led me to wrap it in this code.
I guess since you aren't using any specific front end framework I don't have any real understanding of the lifecycle of the UI elements or order of operations so I'm falling back on patterns that I know are robust when coding straight against the DOM.
Your existing code (models for example) also had logic to deal with this problem (code running before elements exist) but it was coupled to re-fetching data from the server. For example, some of the logic about adding event listeners was tied to the value of isRefresh parameter. I don't have enough context about when or why the code was being called with specific values of isRefresh to feel confident I could lift the same logic and apply it to presets (which have some differences in how they are fetched compared to models).
There was a problem hiding this comment.
I again say go look at the image history code, which already handles this stuff.
If you make up a whole new way to solve a problem, that means if something bugs... there's now two different implementations that need to be separately looked into and separately fixed. If there's a feature to add, there's two different implementations that need to have that separately added to.
As much as possible, new things should either use, or exactly copy, the logic of pre-existing code. Not because one way is necessarily better than the other, but because it makes long term maintenance a lot easier.
There was a problem hiding this comment.
I've done my best to make it similar to image history. But the lifecycles are different and it can't be the same. Presets get initialized differently than image history and as a result the wiring up of the event listeners just can't be done the same way. Hopefully this is close enough to what you are looking for.
|
This is a really odd code approach. Why is it not just identical to image history or model sort code? |
Both Model and Image History use server-side sorting and have an API endpoint dedicated to retrieving them in sorted order. Presets does not use server-side sorting and also does not have a dedicated endpoint (looks like they come over as part of "UserData"). I'm working with what's available. |
Works similar to the way model sorting works. defaults to the existing sort order (which is something similar to but not always "last modified").