mliebelt / pgn-viewer

Simple PGN viewer with the necessary features to display chess games
GNU General Public License v3.0
164 stars 44 forks source link

Fix pieceStyle CSS class manipulation #476

Closed joanlopez closed 12 months ago

joanlopez commented 1 year ago

The current behavior to set the CSS class according to configuration.pieceStyle just adds it as an extra class to the board element:

if (typeof that.configuration.pieceStyle != 'undefined') {
    let bel = document.getElementById(boardId)
    bel.className += " " + that.configuration.pieceStyle
}

However, that's not accurate, because it only works the first time the render function (e.g. pgnView) is called. Then, it keeps adding more classes, leading to an inconsistent behavior that can be reproduced easily in the configuration builder site by switching between different piece styles more than once.

To solve that, I've defined the PieceStyle enum type, but it's not strictly needed. I've also written a small algorithm (setPieceStyleClass function) that probably isn't the most performing solution, but I think it's simple, correct and good enough - Of course, any suggestion is more than welcome! :) cc/ @mliebelt

Note that I also updated the built artifacts for the docs page.

joanlopez commented 1 year ago

The same happens with the configuration.theme, which class is also just added:

divBoard.classList.add(theme)

I'll be happy to create another PR, with the corresponding changes to fix the issue for the configuration.theme once you confirm you like and approve this fix.

Thanks!

joanlopez commented 1 year ago

Also, same behavior can be reproduced for some other attributes, like: mode, layout, etc where the code does the same (just classList.add). Perhaps, it'd better idea to re-build (re-calculate) the classes for every attribute on each execution, and setting them (entirely), instead adding/removing them depending on the selected value.

What do you think?

Thanks!

mliebelt commented 1 year ago

Thank you a lot for providing the fix. I want to check it first locally, and then will create a new version, perhaps tomorrow.

mliebelt commented 12 months ago

Also, same behavior can be reproduced for some other attributes, like: mode, layout, etc where the code does the same (just classList.add). Perhaps, it'd better idea to re-build (re-calculate) the classes for every attribute on each execution, and setting them (entirely), instead adding/removing them depending on the selected value.

I have the same impression, and yes, before creating a new version with just the fix for pieceStyle, why not doing a fix for all the known cases. So I will check it, and see if a "generic" solution may be possible without too much complexity.

Thank you again for bringing that on the table ... I think most of the people will not see that, because the configuration is done most of the time once.

joanlopez commented 12 months ago

Hey @mliebelt, now that this have been merged, are you planning to work on the other issues? Do you want me to give it a try? Thanks!

mliebelt commented 12 months ago

I had hoped to have some time yesterday, but could not manage it. If you want to give it a try, I would really glad about that.