SearchControl: refactor to use InputControl internally #56524
Conversation
|
Flaky tests detected in 0cbc074. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6985950363
|
|
Hey @ciampo do you have any plans to pick this back up? It would be very handy for the data views work. Let me know if I can help at all. |
|
Apologies for not making much progress here, I've been busy with other tasks. I'll try to look into it soon (or ask for help otherwise)! |
This should be acceptable because the original default was even larger.
|
Sounds good. Will test again once changes from #58750 are integrated |
| // @ts-expect-error The `disabled` prop is not yet supported in the SearchControl component. | ||
| // Work with the design team if you need this feature. | ||
| delete restProps.disabled; |
There was a problem hiding this comment.
I added this little safe guard so consumers don't improvise a disabled variant of the SearchControl. Currently, the light gray background of the search box is exactly the same color as a disabled InputControl, so we're going to need some coordinated design work if anyone ever needs a disabled search box.
|
Everything is now up to date and working. The screenshot in the PR description is also updated 👍 |
| className | ||
| ) } | ||
| onChange={ ( nextValue?: string ) => | ||
| onChange( nextValue ?? '' ) |
There was a problem hiding this comment.
Nit: sometimes we can see value || '' and sometimes value ?? ''. While for a normal input control, there shouldn't be a big difference, perhaps we can make these consistent.
There was a problem hiding this comment.
Good idea, I'll unify to ?? since they're closer to the intent. The only value that it would affect here is an empty string, and since we're falling back to an empty string it shouldn't change the outcome. 62b49c0
| ); | ||
| } | ||
| // @ts-expect-error The `disabled` prop is not yet supported in the SearchControl component. | ||
| // Work with the design team if you need this feature. |
There was a problem hiding this comment.
Maybe we should mention the team handle? @WordPress/gutenberg-design?
* Update SearchControl consumers * Revert overrides to New Template Modal
|
While I can't say for sure 100%, I think this PR impacted the "inserter open" performance metric negatively (I'd say around 6 or 7%). Would be cool to try to debug a bit. |
|
Inserter search is impacted as well. For clarity, we have a gap of 24h where the performance tests were not working, so we can't pin point exactly which commit introduced the regression by looking at the graphs https://www.codevitals.run/project/gutenberg I tried looking at the commits and this one is the most suspicious for me (looking at the PR performance job, it seems to confirm, although a single performance job run is not sufficient to judge entirely) |
|
A little bit of performance hit is expected, since we replaced a native |
|
For me, That percentage in the increase is too much for a single component added to the react tree. If we had that performance loss everytime we add a new component to the react tree of the inserter, I think we'll be in a very bad position. Just imagine the number of components we have in the inserter. If one of them is responsible of 7% (something like 12ms or something like that in the graph) that seems like a component that we should investigate |
|
Agreed that we should be mindful of performance. I wonder if this is something we can pinpoint, be it in |
|
I've been doing some isolated perf testing on the components, benchmarking InputControl against TextControl (which is significantly simpler and about 30–40% more performant). Initial experimentation seems to show that the main drag is Emotion, or at least the specific way we use it to add dynamic prop-based styles. I'll see what I can do. |
|
I summarized my findings and next steps here. When we do start migrating, we can start with InputControl since it is widely used, and hopefully we'll be able to validate the gains through Code Vitals. |
|
Thanks for diving into this @mirka I appreciate it. |
Part of #56388
What?
Changes the implementation of the SearchControl so that it uses InputControl internally.
The size variants, which were 48px (default), 40px (next default), and 32px (compact); are now just 40px (default) and 32px (compact).
Why?
To improve cohesion across the app, as discussed in #56388.
Testing Instructions
See Storybook stories for SearchControl.
Please review consumer changes in #58014, which is stacked onto this PR.
Screenshots or screencast