components icon indicating copy to clipboard operation
components copied to clipboard

fix: Pass empty string to onLoadItems after clicking Clear button

Open Al-Dani opened this issue 1 year ago • 1 comments

Description

In property filter when after clicking on Clear button, onLoadItems receives previous filteringText value. This is not expected and different from similar behaviour in autosuggest.

Related links, issue #, if available: AWSUI-59621

How has this been tested?

  • new integ test added
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Al-Dani avatar Sep 27 '24 07:09 Al-Dani

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.36%. Comparing base (ea46162) to head (f25841e). Report is 41 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
+ Coverage   96.34%   96.36%   +0.01%     
==========================================
  Files         780      779       -1     
  Lines       21922    21926       +4     
  Branches     7510     7458      -52     
==========================================
+ Hits        21120    21128       +8     
+ Misses        750      746       -4     
  Partials       52       52              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 27 '24 07:09 codecov[bot]

@Al-Dani , attached a screen recording from main: it looks like the issue or not reproducible or I should use different steps to reproduce it?

https://github.com/user-attachments/assets/73dc2bd0-c72c-45d1-af88-83655e254a4e

pan-kot avatar Oct 22 '24 10:10 pan-kot

@pan-kot no, the issue still persists

There are 2 factors:

  • asyncProperties === true
  • before clicking on the "clear" button, you need to move focus away from the property filter

Al-Dani avatar Oct 23 '24 08:10 Al-Dani

@pan-kot no, the issue still persists

There are 2 factors:

  • asyncProperties === true
  • before clicking on the "clear" button, you need to move focus away from the property filter

I see, but should we consider it a bug? The same (pretty much) behaviour is observed if after the focus is lost the user clicks on the input and then quickly on the clear button. As result there will be two onLoadItems calls, where the first one is probably redundant (unless we want to refresh the data).

I believe that is to be solved on the consumer end. For instance, the calls for the same filteredText and firstPage=true can be ignored (for that the consumer just needs to store the prev request parameters to compare the new request against). Also, it is possible to cancel the previous requests when the new are made. Say, the request for "label = 1" is ongoing and the new request for "" (cleared input) is received: now we can cancel the previous request and no longer relevant and start processing the new one or retrieve the response from cache if available.

pan-kot avatar Oct 24 '24 08:10 pan-kot

I agree, two onLoadItems calls could be (and should be) resolved on the customer end. But we need to provide them with this second call (that has empty string value) in the first place

Al-Dani avatar Nov 29 '24 09:11 Al-Dani