LycheeOrg / Lychee-front

JS implementation of Lychee frontend
https://lycheeorg.github.io/
MIT License
48 stars 53 forks source link

Fix regression bug #296

Closed nagmat84 closed 2 years ago

nagmat84 commented 2 years ago

During review of #291 I noticed that album.buildMessage does not work as expected, because the previous version only searches for the designated album ID in albums.json.albums, i.e. inside the root album. Hence, the confirmation message in #291 did not show the correct title of albums when used inside sub-albums.

I cannot remember to have changed this method during my re-factoring, but it seems so. Now it should be fixed.

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

nagmat84 commented 2 years ago

Yeah, I noticed this the other day but I couldn't remember if it was actually that way before the big merge.

Me neither. But the bug was so obvious to me that I simply assumed that it has to be a regression bug.

Passing just the album IDs [...] is never going to work reliably [...]. We should really be passing the titles themselves, because the caller will pretty much always have access to that info.

Or even better: Pass the album/photo/whatever object itself. If one really wanted to go over the edge, we would even attach the JS object to the DOM when the frontend builds the HTML structure for an album/photo listing. At the moment we simply use the data-attribute of a HTMLElement to set some string ID. The DOM is not limited to adding custom string attributes. (Only our build function is.) In an ideal world we would link our "business" object (i.e. our models) to the HTML element and have it ready in our event handlers and wherever we need them. We would not even need to search for them and as JS does not make deep copies, but stores references this would be super-efficient.

Given the uncertain future prospects of the current front end though, I can't be bothered to fix that.

Me, too.