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

feat(theme): add/update form component themes #9856

Merged

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Mar 31, 2023

Description

This is part of the Implementation of the DS v1

Add or updates theming configs for form components to adhere to the new design system.

  • Adds/updates theming for Radio, Switch, Checkbox, Select (Dropdown), and Input
  • Creates custom component for Input
  • Adds Storybook stories for each component
  • Update other components or pages where necessary for immediate inclusion
    • This is includes adding Select implementation as a replacement for the react-select lib.

Related Issue

Closes #8630

@github-actions github-actions bot added dependencies 📦 Changes related to project dependencies review needed 👀 labels Mar 31, 2023
@TylerAPfledderer TylerAPfledderer force-pushed the feat/new-form-component-themes branch from 55d41bf to 150418c Compare March 31, 2023 04:01
@gatsby-cloud
Copy link

gatsby-cloud bot commented Mar 31, 2023

✅ ethereum-org-website-dev deploy preview ready

@github-actions github-actions bot added the content 🖋️ This involves copy additions or edits label Apr 2, 2023
@TylerAPfledderer TylerAPfledderer force-pushed the feat/new-form-component-themes branch from 3303b67 to 2dbc09e Compare April 13, 2023 03:36
@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review April 14, 2023 00:35
@pettinarip
Copy link
Member

We now use this search "button", and on this PR we have the old one. maybe we should update it if possible

I'll update the branch now but I see the same search button here and in prod 🤔 I'm a bit confused on what you see as the difference.

@pettinarip
Copy link
Member

The read-only should not have hover behavior because it's read-only.

@TylerAPfledderer is that something you know how to add?

@TylerAPfledderer
Copy link
Contributor Author

The read-only should not have hover behavior because it's read-only.

@TylerAPfledderer is that something you know how to add?

@pettinarip The CSS selector variable _notDisabled in the component.utils file can be extended to &:not([data-disabled], [disabled], [data-readonly]) for the control element.

Updates to:

[_notDisabledReadOnly]: {
  "*:hover > &": {
    bg: "primaryHover",
    borderColor: "primaryHighContrast",
  },
},

Are you looking for the cursor to change as well, similar to the disabled radio?

@pettinarip
Copy link
Member

Are you looking for the cursor to change as well, similar to the disabled radio?

I don't think that is necessary for this case. What do you think @nloureiro ?

@nloureiro
Copy link
Contributor

The cursor should not indicate the is an action there.

In the case of the disabled should have the not-allowed

 .not-allowed {
  cursor: not-allowed;
}

@JustinDrake JustinDrake removed their request for review June 14, 2023 15:18
@nloureiro
Copy link
Contributor

After a second review, I want to request a small change on the hover color for the checkbox and radio, the blue was too intense.
changed to body medium

Screen Shot 2023-06-15 09 45 01 AM

@nloureiro
Copy link
Contributor

On the switch, are we using the strong grey as a default?
Maybe that's me but I could not be sure on chromatic preview

Screen Shot 2023-06-15 09 39 22 AM

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip @nloureiro should be good now for another review.

Comment on lines +115 to +116
// TODO: Investigate inconsistency in prop rendering order (possible Chakra bug)
// border: "2px",
Copy link
Contributor Author

@TylerAPfledderer TylerAPfledderer Jun 16, 2023

Choose a reason for hiding this comment

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

For whatever reason, using border and borderColor here works fine for Checkbox, but rendered in reverse order for Radio, cause the borderColor styling to be overriden in the CSS by the border.
image

Now this is probably due to using the default theming which includes an invalid styling (which would be odd for the inconsistency), but I think a refactor in how I am applying these common props might fix it (send these props to the checkbox and then import the checkbox styles to the radio and switch). Otherwise, this might be a compiling bug.

Either way, I think the reorganizing and addressing the prop issue should be saved for a separate PR to get this PR over the finish line... unless you would like for it to be resolved here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks @TylerAPfledderer.

Comment on lines +109 to +116
"*[data-checked]:hover > &": {
bg: "primary.hover",
borderColor: "primary.highContrast",
},
"*:not([data-checked]):hover > &": {
bg: "body.light",
borderColor: "primary.highContrast",
},
Copy link
Member

Choose a reason for hiding this comment

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

@TylerAPfledderer I've updated the hover colors to match the DS. Not sure if there is a more concise way for these selectors but it works as expected.

We want to display primary.hover on checked and body.light on unchecked ones.

borderColor: "disabled",
},
[_notDisabledReadOnly]: {
"*[data-checked]:hover > &, *:not([data-checked]):hover > &": {
Copy link
Member

Choose a reason for hiding this comment

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

@TylerAPfledderer I had to do this to catch the two possible states and override the common styles. The switch is slightly different in terms of hover styles than the radio and checkbox (check DS).

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Looks good. Lots of good stuff here, thanks @TylerAPfledderer

Copy link
Contributor

@nloureiro nloureiro left a comment

Choose a reason for hiding this comment

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

Looks good!!!

@pettinarip pettinarip merged commit a1ffa80 into ethereum:dev Jun 26, 2023
@TylerAPfledderer TylerAPfledderer deleted the feat/new-form-component-themes branch June 26, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits dependencies 📦 Changes related to project dependencies event 📅 This issue or pull request is related to an event listing tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a Chakra Select component
3 participants