`TextField` component updates #16424

georgewrmarshall · 2022-11-09T01:12:30Z

Explanation

What is the current state of things and why does it need to change?
The current TextField component handles the clear button logic but this was actually breaking the component when value was changed outside of the onChange function. It also limits the flexibility of the component.

What is the solution your changes offer and how does it work?

Updating the name of the showClear prop to showClearButton which is more specific
Reducing the logic used inside of the component in favour of letting the engineer handle it for each usecase
Updating the stories and docs to reflect these changes
Updating unit tests to reflect these changes as well as improving the quality of the tests by using userEvent

Screenshots/Screencaps

Before

text.field.before.mov

After

text.field.after.mov

Manual Testing Steps

Go to the latest build of storybook in this PR
Search TextField in the search bar (make sure it's under the Component Library section)
Check the controls and docs

For unit tests

Checkout this branch
Run yarn test:unit:jest ui/components/component-library/text-field/text-field.test.js

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2022-11-09T01:12:42Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

georgewrmarshall · 2022-11-09T01:21:11Z

ui/components/component-library/text-field/text-field.js

-  /**
-   * The props to pass to the icon inside of the close button
-   */
-  clearButtonIconProps: PropTypes.shape(Icon.PropTypes),


Removing clearButtonIconProps because this doesn't exist now that we are using ButtonIcon

NidhiKJha

Just a small nit for tests

ui/components/component-library/text-field/text-field.test.js

metamaskbot · 2022-11-09T23:29:32Z

Builds ready [5cf8830]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2429 ± 163 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	102	229	140	29	14
		domContentLoaded	1846	3193	2396	341	164
		load	1928	3194	2429	339	163
		domInteractive	1846	3193	2396	341	164

Bundle size diffs

background: 0 bytes
ui: 0 bytes
common: 0 bytes

highlights:

storybook

ui/components/component-library/text-field/text-field.stories.js

georgewrmarshall · 2022-11-10T01:38:20Z

ui/components/component-library/text-field/text-field.test.js

-    expect(onClear).toHaveBeenCalledTimes(1);
-    expect(onClick).toHaveBeenCalledTimes(1);
+    await user.click(getByRole('button', { name: /Clear/u }));
+    expect(fn).toHaveBeenCalledTimes(1);
  });
  it('should be able to accept inputProps', () => {
    const { getByRole } = render(


When running these tests locally you may run into this propType issue. It's a known issue and I have an open PR that, with the gracious help of @Gudahtt we are still trying to work through #16078

…ing some tests

…utton tests

metamaskbot · 2022-11-10T17:02:50Z

Builds ready [938e5b5]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2627 ± 175 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	100	826	187	154	74
		domContentLoaded	1964	3307	2604	349	167
		load	1977	3307	2627	365	175
		domInteractive	1964	3307	2603	349	167

Bundle size diffs

background: 0 bytes
ui: 0 bytes
common: 0 bytes

highlights:

storybook

ui/components/component-library/text-field/text-field.stories.js

georgewrmarshall changed the title ~~Updating showClearButton prop name and making component dumber~~ TextField component updates Nov 9, 2022

georgewrmarshall requested review from garrettbear and NidhiKJha November 9, 2022 01:13

georgewrmarshall self-assigned this Nov 9, 2022

georgewrmarshall added area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension IA/NAV labels Nov 9, 2022

georgewrmarshall force-pushed the text-field-updates branch from 879b035 to bcce884 Compare November 9, 2022 01:18

georgewrmarshall commented Nov 9, 2022

View reviewed changes

georgewrmarshall marked this pull request as ready for review November 9, 2022 01:22

georgewrmarshall requested a review from a team as a code owner November 9, 2022 01:22

georgewrmarshall mentioned this pull request Nov 9, 2022

Adding TextFieldSearch component #16296

Merged

16 tasks

NidhiKJha requested changes Nov 9, 2022

View reviewed changes

ui/components/component-library/text-field/text-field.test.js Outdated Show resolved Hide resolved

ui/components/component-library/text-field/text-field.test.js Outdated Show resolved Hide resolved

ui/components/component-library/text-field/text-field.test.js Outdated Show resolved Hide resolved

georgewrmarshall force-pushed the text-field-updates branch from 6b9d3bc to f3cb2c9 Compare November 9, 2022 21:51

georgewrmarshall requested a review from NidhiKJha November 9, 2022 21:52

georgewrmarshall force-pushed the text-field-updates branch from f3cb2c9 to 7153092 Compare November 9, 2022 22:14

garrettbear previously approved these changes Nov 10, 2022

View reviewed changes

georgewrmarshall dismissed garrettbear’s stale review via 7357dff November 10, 2022 01:36

georgewrmarshall force-pushed the text-field-updates branch from 5cf8830 to 7357dff Compare November 10, 2022 01:36

georgewrmarshall commented Nov 10, 2022

View reviewed changes

NidhiKJha previously approved these changes Nov 10, 2022

View reviewed changes

georgewrmarshall added 7 commits November 10, 2022 10:28

Updating showClearButton prop name and making component dumber

f43ff90

Docs update

a467be2

Adding missing proptype

00fc105

Fixing casing on tests

8bc7b49

Replacing clear button placeholder with ButtonIcon and docs update

0b00560

Fixing linting issues

5fe6114

Adding note about controlled only for showClearButton to work and fix…

ab9ae66

…ing some tests

Updating test to include controlled testing setup function for clearB…

938e5b5

…utton tests

georgewrmarshall dismissed NidhiKJha’s stale review via 938e5b5 November 10, 2022 16:28

georgewrmarshall force-pushed the text-field-updates branch from 7357dff to 938e5b5 Compare November 10, 2022 16:28

georgewrmarshall requested review from garrettbear and NidhiKJha November 10, 2022 16:31

NidhiKJha approved these changes Nov 10, 2022

View reviewed changes

theweb3dev approved these changes Nov 10, 2022

View reviewed changes

garrettbear approved these changes Nov 10, 2022

View reviewed changes

garrettbear merged commit 9821c59 into develop Nov 10, 2022

garrettbear deleted the text-field-updates branch November 10, 2022 19:13

github-actions bot locked and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`TextField` component updates #16424

`TextField` component updates #16424

georgewrmarshall commented Nov 9, 2022 •

edited

Loading

github-actions bot commented Nov 9, 2022

georgewrmarshall Nov 9, 2022 •

edited

Loading

NidhiKJha left a comment

metamaskbot commented Nov 9, 2022

georgewrmarshall Nov 10, 2022 •

edited

Loading

metamaskbot commented Nov 10, 2022

TextField component updates #16424

TextField component updates #16424

Conversation

georgewrmarshall commented Nov 9, 2022 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 9, 2022

georgewrmarshall Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

NidhiKJha left a comment

Choose a reason for hiding this comment

metamaskbot commented Nov 9, 2022

highlights:

georgewrmarshall Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Nov 10, 2022

highlights:

`TextField` component updates #16424

`TextField` component updates #16424

georgewrmarshall commented Nov 9, 2022 •

edited

Loading

georgewrmarshall Nov 9, 2022 •

edited

Loading

georgewrmarshall Nov 10, 2022 •

edited

Loading