Skip to content

refactor: simplify filters components#6749

Merged
CommanderStorm merged 10 commits intolouislam:masterfrom
kurama:refactor/filters
Jan 17, 2026
Merged

refactor: simplify filters components#6749
CommanderStorm merged 10 commits intolouislam:masterfrom
kurama:refactor/filters

Conversation

@kurama
Copy link
Copy Markdown
Contributor

@kurama kurama commented Jan 16, 2026

Summary

This pull request restructures the monitor list header to improve the layout and responsiveness of the search bar and filter buttons.

  • Foobar was changed to FooFoo, because ...
Please follow this checklist to avoid unnecessary back and forth (click to expand)
  • ⚠️ If there are Breaking change (a fix or feature that alters existing functionality in a way that could cause issues) I have called them out
  • 🧠 I have disclosed any use of LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • 🔍 Any UI changes adhere to visual style of this project.
  • 🛠️ I have self-reviewed and self-tested my code to ensure it works as expected.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • 🤖 I added or updated automated tests where appropriate.
  • 📄 Documentation updates are included (if applicable).
  • 🧰 Dependency updates are listed and explained.
  • ⚠️ CI passes and is green.

Screenshots for Visual Changes

Before:

image

After:

image
  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).
Event Before After
UP Before After
DOWN Before After
Certificate-expiry Before After
Testing Before After

Copy link
Copy Markdown
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I don't like how this is now always two lines now.

Instead, we should just allow the search bar to shrink a bit on the affected screen sizes.

Image

Plus, on mobile this now looks kind of off.

Image

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

I put it on two lines because, on mobile, the search bar becomes too small if it's to the right of the filters on some screens. The original design was also on two lines, btw. The filter buttons take up a lot of space. I'll try something else.

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

Quick question: why use a simple checkbox? Users don't understand its purpose when they first open the application. Furthermore, it's not possible to select just two groups directly; you have to check the checkbox, uncheck it, and then select the two groups. I find this quite cumbersome for a user. Also, having a "Cancel" button that removes the checkboxes doesn't make sense to me.

We should have: a "Select monitors" button that displays the checkboxes; if this button is not checked, nothing appears; if it is checked, it displays the checkboxes; a "select all" button; and an "actions" button.

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

What do you think about this design:

Step 1: Click the "Select monitors" button to display the checkboxes

image

option to select only 2 items for example

Step 2: Click the "Select all" to select all the monitors

image

You can perform actions by clicking the "Actions" button and/or deselecting all monitors and/or clicking the "Cancel" button to remove the checkboxes.

Is the decision not to select children from the groups intentional?

@CommanderStorm
Copy link
Copy Markdown
Collaborator

I think not having a "select all" button, but this be a checkbox is fairly normal.
It saves space in any sense, which is likely what we want.

Here is for example how github does this -> also just a checkbox

image

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

Yes, but the checkboxes are still there. We can't show/hide them on GitHub or even Gmail. I agree that it saves space.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

How about we move the "Active" Status into the "Status" filter?

For users, these likely feel like the same thing..

@CommanderStorm
Copy link
Copy Markdown
Collaborator

Since this selection feature is fundamentally currently not very usefull (only pause, resume, delete) and does not provide an editing or tagging feature, I think it makes sense to not have it always on and robbing every monitor of vertical space..

@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Jan 16, 2026
@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

How about we move the "Active" Status into the "Status" filter?

For users, these likely feel like the same thing..

That's a good idea, I also think that some people don't make the difference, we should choose a color for "running" and "paused" in this case.

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

Since this selection feature is fundamentally currently not very usefull (only pause, resume, delete) and does not provide an editing or tagging feature, I think it makes sense to not have it always on and robbing every monitor of vertical space..

I agree that it's a good idea not to have the checkboxes visible all the time. I simply modified the functionality to make it more user-friendly. When the user arrives, they see a simple checkbox with no information (knowing that no other checkboxes are displayed), which might confuse them.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

I'd like to not have such a weirdly placed "select monitors" button that would mean we have to have 3 lines for this.

I would like to have 1 line and if we select something, we can have 2 lines.

@CommanderStorm
Copy link
Copy Markdown
Collaborator

And on the "users will find it harder", it would be really nice to have some data on this.
I some time ago did a thread on this, but I lost interest/got burried in other stuff:

Currently, I think we are both talking out of personal prefernce, which is not great.

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

I would like to have 1 line and if we select something, we can have 2 lines.

WDYT ?

image image image

@CommanderStorm
Copy link
Copy Markdown
Collaborator

That works!

I think for extra intuitiveness, we could add a divider and possibly small versions of the play/pause icons that we use elsewere, but I would be fine with it as-is.

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

That works!

I think for extra intuitiveness, we could add a divider and possibly small versions of the play/pause icons that we use elsewere, but I would be fine with it as-is.

Great! I'll push that.
I tried using the separator, but it didn't really convince me 😅

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

Updated !
image

image

@kurama kurama marked this pull request as ready for review January 16, 2026 11:55
Copilot AI review requested due to automatic review settings January 16, 2026 11:55

This comment was marked as spam.

Copy link
Copy Markdown
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

This seems to work much nicer than previus work.

I have three changes:

  • on mobile, this seems rather squished. I think we still need to place the search bar on top there.
  • I think the way the play/pause is integrated needs a few items tewaked. For example, they cannot appear here
    Image
  • selecting them makes the text of the Play/pause flicker

@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 16, 2026

I've pushed two fixes, but I'm not seeing the text blinking. Could you retest and let me know?

@CommanderStorm CommanderStorm added this to the 2.1.0 milestone Jan 17, 2026
@CommanderStorm CommanderStorm added the releaseblocker 🚨 blocking bugs encountered with a new release label Jan 17, 2026
Copy link
Copy Markdown
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Nice work, thank you ❤️

The issue that I was talking about with blinking is that in the light mode, the selection color is white.. on a background which is slightly off-white.

It is a pre-existing issue though, so lets merge this improvement.
Thank you ^^

@CommanderStorm CommanderStorm enabled auto-merge (squash) January 17, 2026 12:33
@CommanderStorm CommanderStorm removed the pr:please address review comments this PR needs a bit more work to be mergable label Jan 17, 2026
@CommanderStorm CommanderStorm merged commit 18331ea into louislam:master Jan 17, 2026
24 checks passed
@kurama
Copy link
Copy Markdown
Contributor Author

kurama commented Jan 17, 2026

Always a pleasure! Thank you! ❤️

@louislam
Copy link
Copy Markdown
Owner

Not sure if #6820 is related to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releaseblocker 🚨 blocking bugs encountered with a new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants