[MPDX-7837] Adds close adornment to base search text field. #869

wjames111 · 2024-02-02T21:17:08Z

Description

Search needs x to clear on Contacts and Tasks. Ticket #MPDX-7837

Checklist:

I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
I have requested a review from another person on the project

aws-amplify-us-east-1 · 2024-02-02T21:29:46Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-869.d3dytjb8adxkk5.amplifyapp.com

canac

The clear button works but is browser-dependent and doesn't really match our theme. If Scott is OK with that, then this is good. Otherwise, you could use an input adornment to add an x button that matches the MUI styles better. https://mui.com/material-ui/react-text-field/#input-adornments

canac

Nice! Can you figure out how to keep the width of the input from changing when the close button shows and hides? Maybe giving the <SearchInput> an explicit width would help.

canac · 2024-02-14T14:17:48Z

src/components/common/SearchBox/SearchBox.tsx

@@ -55,6 +74,18 @@ export const SearchBox: React.FC<SearchBoxProps> = ({
            )}
          </InputAdornment>
        ),
+        endAdornment: (
+          <InputAdornment position="end">
+            {isClearSearchVisible && (


Instead of using a separate state variable for this, I would just hide it when currentSearchTerm === ''.

Nice, yeah that makes better sense.

canac · 2024-02-14T15:16:14Z

@wjames111 It would also be great to add a simple test case for the new functionality.

…s simple test for SearchInput close button.

wjames111 · 2024-02-14T16:19:40Z

@wjames111 It would also be great to add a simple test case for the new functionality.

@canac Sounds good! I added a check for onClick to clear the textbox, and added a width so the textfield doesn't expand. From what I tested the width should be comparable to the size it was before on all screen sizes. Let me know if there's anything else before I mark it ready for QA.

canac

Great work with the sizing and the tests!

canac · 2024-02-14T18:15:35Z

src/components/common/SearchBox/SearchBox.tsx

+          <InputAdornment position="end">
+            {currentSearchTerm && (
+              <IconButton
+                onClick={handleClearSearch}


I think you need to use handleOnChange here so that onChange gets called too. Currently, clicking x clears the search input but doesn't update the actual search filter on the page.

canac · 2024-02-14T18:16:31Z

src/components/common/SearchBox/SearchBox.test.tsx

@@ -41,6 +41,7 @@ it('triggers onChange', async () => {
  );

  const textbox = getByRole('textbox');
+  // const searchInputCloseButton = getByTestId('SearchInputCloseButton');


Let's remove this if it's not needed.

Oops, my bad.

canac · 2024-02-14T18:17:07Z

src/components/common/SearchBox/SearchBox.test.tsx

@@ -49,4 +50,8 @@ it('triggers onChange', async () => {
  await new Promise((resolve) => setTimeout(resolve, 300));


Instead of an explicit wait here, you can do await waitFor(() => expect(onChange).toHaveBeenCalledWith(inputText));

canac · 2024-02-14T18:18:20Z

src/components/common/SearchBox/SearchBox.test.tsx

+
+  userEvent.click(getByTestId('SearchInputCloseButton'));
+
+  expect(textbox).toHaveValue('');


Let's also test that onChange gets called with '' to prove that the bug I mentioned below gets fixed.

…eck for bug.

canac

It looks good! 🎉 I left one suggestion for the test. I would also rebase these commits down to just one at some point before merging. Then you can add the "On Staging" label if you haven't yet and move the ticket over.

src/components/common/SearchBox/SearchBox.test.tsx

Co-authored-by: Caleb Cox <canac@users.noreply.github.com>

dr-bizz

Looks great. I just had one comment to make the simplify the code.

src/components/common/SearchBox/SearchBox.tsx

dr-bizz

Great work! 🔥

You need to merge this branch into staging locally and push it up to staging, then you can retry the staging test on this PR.

Staging is a little broken these days. I'm trying to fix it ATM.

wjames111 · 2024-02-22T19:51:07Z

This has been added to Staging manually. Still showing a merge conflict for some reason.

Adds type seach to base search text field.

a3d1e9c

wjames111 added the On Staging Will be merged to the staging branch by Github Actions label Feb 2, 2024

wjames111 changed the title ~~Adds type seach to base search text field.~~ Adds type attribute to base search text field. Feb 2, 2024

wjames111 changed the title ~~Adds type attribute to base search text field.~~ [MPDX-7837] Adds type attribute to base search text field. Feb 12, 2024

canac self-requested a review February 13, 2024 21:13

canac approved these changes Feb 13, 2024

View reviewed changes

Adapts mui adornments for clear search icon.

dd5a408

canac requested changes Feb 14, 2024

View reviewed changes

Adds width to searchInput base component, removes unneaded state, add…

e8173f8

…s simple test for SearchInput close button.

wjames111 changed the title ~~[MPDX-7837] Adds type attribute to base search text field.~~ [MPDX-7837] Adds close adornment to base search text field. Feb 14, 2024

canac requested changes Feb 14, 2024

View reviewed changes

Fixes base search input bug, not removing results. Updates test to ch…

40d7f82

…eck for bug.

canac approved these changes Feb 14, 2024

View reviewed changes

src/components/common/SearchBox/SearchBox.test.tsx Outdated Show resolved Hide resolved

Update src/components/common/SearchBox/SearchBox.test.tsx

60d0e3d

Co-authored-by: Caleb Cox <canac@users.noreply.github.com>

dr-bizz reviewed Feb 16, 2024

View reviewed changes

src/components/common/SearchBox/SearchBox.tsx Show resolved Hide resolved

Replaces CloseButtonIcon with Close and uses sx for theme text color.

597530d

dr-bizz approved these changes Feb 16, 2024

View reviewed changes

wjames111 added On Staging Will be merged to the staging branch by Github Actions and removed On Staging Will be merged to the staging branch by Github Actions labels Feb 22, 2024

wjames111 merged commit 5f96e71 into main Feb 22, 2024
17 of 19 checks passed

wjames111 deleted the 7837-clear-search-button branch February 22, 2024 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MPDX-7837] Adds close adornment to base search text field. #869

[MPDX-7837] Adds close adornment to base search text field. #869

wjames111 commented Feb 2, 2024 •

edited

Loading

aws-amplify-us-east-1 bot commented Feb 2, 2024

canac left a comment

canac left a comment

canac Feb 14, 2024

wjames111 Feb 14, 2024

canac commented Feb 14, 2024

wjames111 commented Feb 14, 2024 •

edited

Loading

canac left a comment

canac Feb 14, 2024

canac Feb 14, 2024

wjames111 Feb 14, 2024

canac Feb 14, 2024

canac Feb 14, 2024

canac left a comment

dr-bizz left a comment

dr-bizz left a comment

wjames111 commented Feb 22, 2024

		@@ -49,4 +50,8 @@ it('triggers onChange', async () => {
		await new Promise((resolve) => setTimeout(resolve, 300));


		userEvent.click(getByTestId('SearchInputCloseButton'));

		expect(textbox).toHaveValue('');

[MPDX-7837] Adds close adornment to base search text field. #869

[MPDX-7837] Adds close adornment to base search text field. #869

Conversation

wjames111 commented Feb 2, 2024 • edited Loading

Description

Checklist:

aws-amplify-us-east-1 bot commented Feb 2, 2024

canac left a comment

Choose a reason for hiding this comment

canac left a comment

Choose a reason for hiding this comment

canac Feb 14, 2024

Choose a reason for hiding this comment

wjames111 Feb 14, 2024

Choose a reason for hiding this comment

canac commented Feb 14, 2024

wjames111 commented Feb 14, 2024 • edited Loading

canac left a comment

Choose a reason for hiding this comment

canac Feb 14, 2024

Choose a reason for hiding this comment

canac Feb 14, 2024

Choose a reason for hiding this comment

wjames111 Feb 14, 2024

Choose a reason for hiding this comment

canac Feb 14, 2024

Choose a reason for hiding this comment

canac Feb 14, 2024

Choose a reason for hiding this comment

canac left a comment

Choose a reason for hiding this comment

dr-bizz left a comment

Choose a reason for hiding this comment

dr-bizz left a comment

Choose a reason for hiding this comment

wjames111 commented Feb 22, 2024

wjames111 commented Feb 2, 2024 •

edited

Loading

wjames111 commented Feb 14, 2024 •

edited

Loading