-
Notifications
You must be signed in to change notification settings - Fork 10
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
SearchField: Add validation props and refine states #2363
base: main
Are you sure you want to change the base?
Conversation
… on TextField styles
🦋 Changeset detectedLatest commit: d2a4cd5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +85 B (+0.08%) Total Size: 101 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-wqaepcqqyj.chromatic.com/ Chromatic results:
|
ba4067b
to
41702d7
Compare
… some issues with text alignment
41702d7
to
5f9f586
Compare
@@ -146,7 +174,7 @@ const SearchField: React.ForwardRefExoticComponent< | |||
|
|||
// @ts-expect-error [FEI-5019] - TS2322 - Type '() => JSX.Element | null' is not assignable to type '() => ReactElement<any, string | JSXElementConstructor<any>>'. | |||
const maybeRenderClearIconButton: () => React.ReactElement = () => { | |||
if (!value.length) { | |||
if (!value.length || disabled) { |
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.
Now that we have the Search Field Variants stories, addressed a small bug where we were showing the clear button when the field is disabled!
@@ -223,18 +255,11 @@ const styles = StyleSheet.create({ | |||
margin: 0, | |||
position: "absolute", | |||
right: 0, | |||
":hover": { |
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.
This was causing an issue where the focus outline would not be shown if the clear button was hovered and focused. Also, the hover state of the clear button doesn't have a border so this isn't needed!
}, | ||
inputStyleReset: { | ||
display: "flex", | ||
flex: 1, | ||
"::placeholder": { |
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.
We can rely on the styles from TextField so it is consistent!
@@ -167,8 +195,14 @@ const SearchField: React.ForwardRefExoticComponent< | |||
<View onClick={onClick} style={[styles.inputContainer, style]}> | |||
<PhosphorIcon | |||
icon={magnifyingGlassIcon} | |||
size="medium" | |||
color={color.offBlack64} | |||
size="small" |
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.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (33b95e2) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2363" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2363 |
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.
This is looking great! left a couple of nit/non-blocking suggestions before giving final approval.
padding: spacing.medium_16, | ||
}, | ||
scenarios: { | ||
display: "flex", |
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.
super-nit: View
provides flex
by default.
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.
Removed! (🦸 super-nit 😄)
/> | ||
{(errorMessage || args.error) && ( | ||
<> | ||
<Strut size={spacing.xSmall_8} /> |
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.
suggestion: I guess you could also set gap: spacing.xSmall_8
in the parent View
container. This approach works well too if you want to keep it.
On a side note, I feel that Strut
and Spring
were created in times where we didn't have so many CSS flex and grid goodies to solve these kind of issues. The more I think, the more I'm convinced that we should recommend using gap
instead of these components. What do you think about that? (not that we have to migrate existing uses, but more that we should recommend this approach from now on if it makes sense).
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.
That makes sense! I wonder if it would be helpful to add a gap
or space
prop to View
. What do you think?
Also, is it preferred we inline the aphrodite styles or create styles in a stylesheet for things like this?
<View style={{ gap: spacing.xSmall_8 }} />
or
<View style={styles.container} />
...
const styles = StyleSheet.create({
container: {
gap: spacing.xSmall_8,
}
});
Is one more performant? One nice thing about inlining the styles is that if the styling gets removed, it's removed! Unlike the other way, there's a chance we forget to remove the unused style from the stylesheet! This may also change depending on our css approach, curious if anyone has thoughts on this :)
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.
The inline approach sure reminds me of Tailwind! It puts the styles closer to their usage. One downside is the code is less portable, whereas classes can be centralized and reused.
return null; | ||
} | ||
|
||
return ( | ||
<IconButton | ||
icon={xIcon} | ||
size="xsmall" |
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.
I also updated the dismiss icon styling (bold icon, smaller, spacing!). Thanks for catching this @jandrade!
Since IconButton is smaller now, this addresses the issue where the focus outline on the dismiss button can be cropped out if an ancestor element has overflow: hidden
(see Chromatic comment)
The hit area seems too small though for accessibility. I wonder if all sizes of the IconButton component should all have the same minimum hit area?
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.
I think we should probably use size="small"
instead, as it keeps the same icon size (16x16) but the hit area is slightly bigger (32 instead of 24), which would improve a11y in that regard. We'd still fail to achieve AAA, but I think it would be a good compromise for the mid-term.
And I like the suggestion about revisiting the hit areas of all IconButton
variants. I think this is something worth exploring and documenting in the WB audit that we are doing for the new experience.
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.
size="small"
for IconButton uses a 24x24 icon, which is different from size="small"
for PhosphorIcon! I'll update this to size="small"
and have the icon be a bit bigger so it is more usable!
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.
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.
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.
Ahh yes that is confusing! I added a note in WB-1792 to document tips for developers around icons once we work with design to make the translation easier!
I also updated the audit spreadsheet to include notes about the IconButton hit area and IconButton/PhosphorIcon size consistency!
…e regular icon again so the bold icon isn't too prominent
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 awesome! thanks for adding these changes and for getting more consistency across our form fields 👏 🚀
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.
This seems like a really great change, nice work @beaesguerra!
Summary:
To make the SearchField component more consistent with other form field components like TextField and TextArea, we add validation related props to the component. See LabeledField Implementation Spec for more details!
The changes include:
Issue: WB-1761
Test plan:
?path=/docs/packages-searchfield--docs#error
?path=/docs/packages-searchfield--docs#validation
?path=/docs/packages-searchfield--docs
)?path=/docs/packages-searchfield-all-variants--docs
)