Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
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
GH-1975 Theme Framework & GH-1972 Palm Theme #517
GH-1975 Theme Framework & GH-1972 Palm Theme #517
Changes from 1 commit
317ab64f050b83e8d7f92f9b1546a9a12aafc6149405a37b30e0beef8a66ff91ff8eb460cfc9105f8718824b2a3d36fd1a260c62786606431b2cb005183da745f95623e3b4fa536dd74d9e06f5File filter
Jump to
wlycdgrMar 27, 2020
Member
Try to resolve this using the suggestions at https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/label-has-associated-control.md if you haven't yet
benstrumeyerMar 31, 2020
Author
Contributor
Definitely will try to resolve those before resorting to eslint, I think I mistakenly put that here since there are no labels
wlycdgrMar 27, 2020
Member
Destructure to clean up a lil
const { checked, handleClick } = props;, etcbenstrumeyerMar 31, 2020
Author
Contributor
Destructured!
wlycdgrMar 27, 2020
Member
add
checkedbenstrumeyerMar 31, 2020
Author
Contributor
Added checked!
wlycdgrMar 27, 2020
Member
buttonssuggests that the items are button objects/components. Use a name that better reflects the nature of the (on/off bool) elements.benstrumeyerMar 31, 2020
•
edited
Author
Contributor
Changed to buttonState! Actually, removed altogether per Ethan's comment:
It looks like this is the only place that you're using the local state in this component. You can instead just pass the props to the
<RadioButton />component like this:Then, you can remove all of the code from
componentDidMountandRadioButtonGroup.handleClickand not have to worry about local state.wlycdgrMar 27, 2020
Member
Since we only use the
textproperty of these objects for the individual button labels, let's pass in the labels only, as an array of strings. Not only do we avoid passing unused data that way but we also make the component more flexible, since it will no longer be expeting an object prop with a Themes-view-specific shape.benstrumeyerMar 31, 2020
Author
Contributor
So reusable. Passed in labels for flexibility
wlycdgrMar 27, 2020
Member
Update to
dark-blue-themehere toowlycdgrMar 27, 2020
Member
And let's be consistent with the naming -
default-theme,palm-theme,leaf-theme, etcbenstrumeyerMar 31, 2020
Author
Contributor
Ah I wanted to do that. It's my understanding that these theme names rely on the account project for API calls and we don't want to update these for now, however we have a ticket in the backlog for it https://cliqztix.atlassian.net/browse/GH-1974