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

Implement replace_css_class action #116

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

leonvogt
Copy link
Sponsor Contributor

@leonvogt leonvogt commented Oct 18, 2023

This PR implements the replace_css_class action.

I ended up not using the suggested replace() function from the issue, because I didn't saw a way to use it with multiple classes.
The downside of my implementation is, it could change the order of the classes.
But I think that shouldn't be a big problem, since the order of the classes shouldn't matter.

closes #52

@marcoroth
Copy link
Owner

Hey @leonvogt, thanks for this pull request!

I'm wondering if we should just limit this action to one class in the from and to.

The classList.replace() function has some nuances which are currently not respected in this implementation. While in most cases the outcome is/should be the same, the classList.replace() only replaces the class when the element has the from class. Your implementation would add the to class in any case.

There might be an opportunity in TurboPower Rails to make it easier to use and to account for multiple classes. In that case the Rails helper could just return multiple <turbo-stream action="replace_css_class"></turbo-stream> tags in the same string. Or alternatively, we could provide another replace_css_classes that more or less does what you were doing here.

Technically it would be fine to implement it like you have it here, but since we are trying to stay as close as possible to the native web APIs I think we should change the replace_css_class to just take in one class.

@leonvogt
Copy link
Sponsor Contributor Author

leonvogt commented Nov 3, 2023

Hey @marcoroth, thanks for the feedback!
I think your concerns are very reasonable. I didn't thought about the fact that the replace method only replaces the class if the element has the from class.
I've updated the PR to only replace one class at a time.

Copy link
Owner

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the pull request, @leonvogt!

@marcoroth marcoroth merged commit 18a491d into marcoroth:main Nov 5, 2023
5 checks passed
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.

Implement replace_css_class action
2 participants