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

Remove non-effective color prop in ToggleButton #399

Merged

Conversation

chrismayer
Copy link
Collaborator

@chrismayer chrismayer commented Jun 6, 2024

This removes non-effective color prop definition in the ToggleButton component.

Copy link
Collaborator

@sronveaux sronveaux left a comment

Choose a reason for hiding this comment

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

Hi @chrismayer,

Just made the test and have to say that only one of the rules you removed is truly non-effective.

I prefer to let you see whether this should be amended or whether it should stay the same... but in this case, PR name should be changed as one of the removed lines IS still effective...

@fschmenger
Copy link
Collaborator

Maybe add @ntma as a reviewer and wait until he is around?
A lot of work and testing went into theming, so rather be sure there are no unintended side effects.

@chrismayer chrismayer force-pushed the rm-non-effective-color-props branch from 19b31a9 to 1a392b9 Compare June 10, 2024 14:37
@chrismayer chrismayer changed the title Remove non-effective color props in ToggleButton Remove non-effective color prop in ToggleButton Jun 10, 2024
@chrismayer
Copy link
Collaborator Author

I just added the correction due to review comment by @sronveaux. Could you please give it another view and approve if you're happy now?

@sronveaux sronveaux self-requested a review June 10, 2024 15:06
Copy link
Collaborator

@sronveaux sronveaux left a comment

Choose a reason for hiding this comment

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

Looks perfect from my side and is ready to merge !

I let you appraise whether there is a chance to get @ntma comment on this... I suppose this can be changed again in the future in that case !

@chrismayer
Copy link
Collaborator Author

Thanks for your additional review @sronveaux. I am going to merge now, since I don't expect any problems by this changeset. @ntma If I am wrong, please comment here (even after merging) and I'll happily come up with separate PR.

@chrismayer chrismayer merged commit 52c22f0 into wegue-oss:master Jun 12, 2024
1 check 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.

3 participants