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

Fix pieceStyle CSS class manipulation #476

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Oct 30, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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.

@mliebelt mliebelt merged commit a032b38 into mliebelt:main Oct 31, 2023
2 checks passed
@joanlopez
Copy link
Contributor Author

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
Copy link
Owner

mliebelt commented Nov 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants