Skip to content
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

Fix:issue#1216 allow the search filters to be cleared or reset to the default #1292

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shubhit7
Copy link
Contributor

Fixes #1216

  • Change owner filter, from FormSelect to Select next component of patternfly alpha release. Old Select has been deprecated.
  • Change the search field beside owner filter, from TextInput to SearchInput.

@shubhit7
Copy link
Contributor Author

I figured out how we can use deprecated Select.
@KKoukiou Shall we use deprecated Select or Select Next ?

@KKoukiou
Copy link
Contributor

I figured out how we can use deprecated Select. @KKoukiou Shall we use deprecated Select or Select Next ?

Either of the two works for now. Tending towards the deprecated one, so that we can move all Selects at once to the new.

@shubhit7 shubhit7 marked this pull request as ready for review May 19, 2023 09:10
@KKoukiou KKoukiou self-requested a review May 22, 2023 12:17
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with the search input adoption - but totally disagree with adopting the typeahead for the system/user filter. Logs page uses the typeahead in a case where there are hunderds of select options, not 3 like here.
I know it's suggested in #1216 (comment) but it's just not correct design wise..

Please undo the select part, and keep the SearchInput. Let's also add a test for the new functionality and it's good to go.

src/ContainerHeader.jsx Outdated Show resolved Hide resolved
@shubhit7 shubhit7 marked this pull request as draft May 24, 2023 10:40
@KKoukiou KKoukiou force-pushed the fix-Issue#1216-Allow-the-search-filters-to-be-cleared-or-reset-to-the-default branch from 41102b5 to ba80941 Compare June 6, 2023 08:52
@KKoukiou KKoukiou marked this pull request as ready for review June 6, 2023 08:52
Comment on lines +30 to +31
onChange={(_event, value) => handleFilterChanged(value)}
onClear={() => handleFilterChanged('')} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test. Details

@jelly
Copy link
Member

jelly commented Jun 22, 2023

PR looks good from my side but the tests fail everywhere, it looks like you will need to adjust how test for the value of the search input:

wait_js_cond(ph_has_val("#containers-filter","pod-1")): Uncaught (in promise) Error: #containers-filter does not have a value

@shubhit7
Copy link
Contributor Author

PR looks good from my side but the tests fail everywhere, it looks like you will need to adjust how test for the value of the search input:

wait_js_cond(ph_has_val("#containers-filter","pod-1")): Uncaught (in promise) Error: #containers-filter does not have a value

You are right, using the SearchInput component changes the structure of dom elements compared to using simple TextInput component. Now the input box will be accessible by selector "#containers-filter input".

wait_js_cond(ph_has_val("#containers-filter input","pod-1")) should not throw any exception. I am not able to get hands on test suite setup.

@jelly
Copy link
Member

jelly commented Jul 3, 2023

PR looks good from my side but the tests fail everywhere, it looks like you will need to adjust how test for the value of the search input:

wait_js_cond(ph_has_val("#containers-filter","pod-1")): Uncaught (in promise) Error: #containers-filter does not have a value

You are right, using the SearchInput component changes the structure of dom elements compared to using simple TextInput component. Now the input box will be accessible by selector "#containers-filter input".

wait_js_cond(ph_has_val("#containers-filter input","pod-1")) should not throw any exception. I am not able to get hands on test suite setup.

To get the test running should be fairly easy:

TEST_OS=fedora-38 make vm
TEST_SHOW_BROWSER=1 ./test/check-application -vst TestApplication.testPods

@jelly
Copy link
Member

jelly commented Aug 23, 2023

Hey @shubhit7 do you still intend to work on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allows the search filters to be cleared or reset to the default
4 participants