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

feat(recordings): UI improvements for recording filters #1335

Merged
merged 30 commits into from
Sep 6, 2024

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Aug 24, 2024

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Related to #1303
Depends on #1334

Description of the change:

  • Implemented some ideas to improve filter toolbar in Recording table.
  • Fixed the typeahead toggle's focus. In PF5, first list item is focused by default, which causes the text input to lose focus, thus typing does not register any characters.

Minor changes

Changes Description Screenshot
Use "read" style for count badge (i.e. grey background) Following PF5 examples. Mostly, to avoid potential misunderstanding for an unread action image

Major changes

Changes Description Screenshot
Duration Filter should allow range selection and have a search button for confirmation. "Enter" key is still supported Range selection allows more flexible filter options. Also, it was previously hard to figure out the "Enter" key is for confirmation. The continuous checkout is moved down to make space & it seems similar to that in the Recording Create Form image
Combined view for date and time picker. 24-hr mode switch is now replaced with a toggle group (to represent modes, for example, YAML/JSON view). Some border is fixed to match PF styles A single view reduces the steps required to select a datetime (i.e. avoid switching tabs back and forth) image

@tthvo tthvo added feat New feature or request safe-to-test labels Aug 24, 2024
@tthvo tthvo requested a review from a team August 26, 2024 23:55
@tthvo tthvo marked this pull request as ready for review August 26, 2024 23:55
@tthvo tthvo force-pushed the pf5-recording-filters branch from e21f3ad to 7773691 Compare September 3, 2024 20:31
@github-actions github-actions bot removed the dependent label Sep 3, 2024
Copy link

github-actions bot commented Sep 3, 2024

This PR/issue depends on:

@tthvo tthvo force-pushed the pf5-recording-filters branch from 7773691 to e436e04 Compare September 3, 2024 20:34
@tthvo tthvo force-pushed the pf5-recording-filters branch from e436e04 to 04f3d9a Compare September 4, 2024 15:11
@andrewazores
Copy link
Member

andrewazores commented Sep 6, 2024

Awesome work on the filter redesign here, these look and feel great to use!

One behaviour I think might be worth changing:

image

image

To me, it "feels" like a Continuous recording should be included in a "duration longer than X" filter, because Continuous ~= "Infinity".

I can get the result I want here by putting in the "duration longer than X" filter plus ticking the Continuous checkbox, but somehow this doesn't feel quite intuitive to me.

@andrewazores
Copy link
Member

I can get the result I want here by putting in the "duration longer than X" filter plus ticking the Continuous checkbox, but somehow this doesn't feel quite intuitive to me.

Maybe hitting Enter/clicking Search with only a duration lower bound should auto-tick the Continuous checkbox?

@tthvo
Copy link
Member Author

tthvo commented Sep 6, 2024

Maybe hitting Enter/clicking Search with only a duration lower bound should auto-tick the Continuous checkbox?

I think this idea make sense. Just that we have a filter value of "Continuous", which that checkbox handles. If this above idea is implemented, when uncheck the box will now also remove the filters without upper bound? Maybe that would not be okay?

@tthvo tthvo force-pushed the pf5-recording-filters branch from ea48266 to 6dbc7e3 Compare September 6, 2024 16:55
@tthvo
Copy link
Member Author

tthvo commented Sep 6, 2024

Maybe hitting Enter/clicking Search with only a duration lower bound should auto-tick the Continuous checkbox?

I think this idea make sense. Just that we have a filter value of "Continuous", which that checkbox handles. If this above idea is implemented, when uncheck the box will now also remove the filters without upper bound? Maybe that would not be okay?

The latest commit should have this behaviour. The checkbox is stucked when there is a filter without an upper bound...

@andrewazores
Copy link
Member

I think something about the LocalStorage/Redux changes may have gone wrong. This happens after clearing site data or using a private browsing window (Firefox).

Screenshot_2024-09-06_13-07-34

@tthvo
Copy link
Member Author

tthvo commented Sep 6, 2024

I think something about the LocalStorage/Redux changes may have gone wrong. This happens after clearing site data or using a private browsing window (Firefox).

Oh hmmm, I don't see that on my side....Is there anything on the console that might give some insights? It does look like something with webpack when it builds the web assets?

@tthvo
Copy link
Member Author

tthvo commented Sep 6, 2024

Maybe hitting Enter/clicking Search with only a duration lower bound should auto-tick the Continuous checkbox?

I think this idea make sense. Just that we have a filter value of "Continuous", which that checkbox handles. If this above idea is implemented, when uncheck the box will now also remove the filters without upper bound? Maybe that would not be okay?

The latest commit should have this behaviour. The checkbox is stucked when there is a filter without an upper bound...

For this, the filtering behaviour is as intended. But maybe leave out the visual part where the Continuous checkbox is auto-tick?

@andrewazores
Copy link
Member

Whoops, my bad. Ignore me. I forgot to update dependencies again after switching back to testing this branch! It's all good once I make sure I'm actually building with pf5 haha.

@andrewazores
Copy link
Member

The latest commit should have this behaviour. The checkbox is stucked when there is a filter without an upper bound...

For this, the filtering behaviour is as intended. But maybe leave out the visual part where the Continuous checkbox is auto-tick?

Hmm. Yes, it's working the way my intuition likes it now, but it does feel like a bug that the box can't be unticked. Maybe disabling it and adding a hover tooltip to explain... ?

@tthvo
Copy link
Member Author

tthvo commented Sep 6, 2024

Hmm. Yes, it's working the way my intuition likes it now, but it does feel like a bug that the box can't be unticked. Maybe disabling it and adding a hover tooltip to explain... ?

Actually, that sounds good! How about now?

locales/en/public.json Outdated Show resolved Hide resolved
@andrewazores andrewazores merged commit 8a2cbb9 into cryostatio:pf5 Sep 6, 2024
12 of 18 checks passed
@tthvo tthvo deleted the pf5-recording-filters branch September 6, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants