-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
Reuse grapher checkboxes in site #2888
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
@danyx23 you recently created a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
I think it would make sense to move the checkbox into the components
package
Agreed that grapher.scss
should be cleaned up :)
onChange: () => any | ||
label: string | (() => React.ReactNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of interest, why is the label a function and not a plain react node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @sophiamersmann!
Yes you're right, we don't need the render prop here 👍
c9c9dd5
to
895577e
Compare
For #2890, we need a redesigned checkbox.
this PR makes the recently redesigned checkbox available to the site context.
I'm adding the capability to render nodes in addition to strings in the label. This is to cater for links in the label ("list of donors")
there is a global grapher style which takes care of the checkbox hover state. This style is not available in the site context. I chose to add it to the component itself to make it self-contained. There are other places which rely on the global style, and would require a deeper refactor if we wanted to get rid of it. Here is a demo of the problem this fixes:
Screen.Recording.2023-11-02.at.14.08.57.mov