Skip to content

Conversation

@danielbeardsley
Copy link
Member

It took a long time to get this to behave intuitively. In short, it took
a long to spec it out. How to have it behave when you click one of the
values, how to represent the default state, ...

The feature as implemented:

  • FilterMenu has a new prop defaultExcludedValues: string[]
  • Without defaultExcludedValues set, it operates exactly as it does now
  • When set, the UI has one new "showDefault" state in addition to
    existing "showAll"
  • In "showDefault", Pulls matching one of the excluded values are hidden
    (these excluded values appear de-selected in the UI)
    • One click "selects" them and leaves "showDefault" mode
  • "Show All" overrides the "default exclusions" and shows everything

Closes #414

It took a long time to get this to behave intuitively. In short, it took
a long to spec it out. How to have it behave when you click one of the
values, how to represent the default state, ...

The feature as implemented:

* FilterMenu has a new prop defaultExcludedValues: string[]
* Without defaultExcludedValues set, it operates exactly as it does now
* When set, the UI has one new "showDefault" state in addition to
  existing "showAll"
* In "showDefault", Pulls matching one of the excluded values are hidden
  (these excluded values appear de-selected in the UI)
  * One click "selects" them and leaves "showDefault" mode
* "Show All" overrides the "default exclusions" and shows everything
Provide a way to hide a repo by default from the config by adding an
option to the config. We have to pass the repo specs part of the config
around more, and transform these `{org}/{repo}` names into just the
{name} portion for the filter, but this works for now.
@danielbeardsley
Copy link
Member Author

CC @sterlinghirsh

jarstelfox
jarstelfox previously approved these changes Jan 16, 2025
Copy link
Contributor

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

jarstelfox
jarstelfox previously approved these changes Jan 16, 2025
Copy link
Contributor

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

@sterlinghirsh
Copy link
Member

Glad to see this happening. I think ifixit-schooner-* and PD-Test should probably be added to the default exclusion list.

It used to be that clicking a single repo would show just that repo, which is usually what I want from a menu like that, but now since they all start checked, clicking a single repo hides it. That is reasonably intuitive when they start checked, though there's no way to select none, so if I do just want to see a single repo, it's like 15 clicks. I think if there was a select none option, that would be sufficient. Datadog handles this with a little button on each menu option that says "only", but that might be overkill for this. Maybe also if it could remember my selection in localstorage or something, that would be good.

I think that Select All, Default, and None should be buttons (actions) rather that checkboxes.

In any case I think this is a great step in the right direction, so thanks for working on it.

@danielbeardsley
Copy link
Member Author

Maybe also if it could remember my selection in localstorage or something, that would be good.

All selections are stored in the url. Bookmarking works well and allows several different configurations.

I think that Select All, Default, and None should be buttons (actions) rather that checkboxes.

That's not a bad idea. I'll try that out.

Instead of "Show All" being a state, it's now just a button.
Instead of a click isolating that option when in Show All mode, it will
simply uncheck it. This adds an "only" option to every menu item to
replace the old functionality.
@danielbeardsley
Copy link
Member Author

I think that Select All, Default, and None should be buttons (actions) rather that checkboxes.

Done! And I've added an "only" instead of "None"

@kthaler kthaler self-assigned this Jan 16, 2025
@kthaler kthaler added the QAing Under QA team review label Jan 16, 2025
@kthaler
Copy link
Contributor

kthaler commented Jan 16, 2025

QA 💎 - We can now exclude repos from pulldasher by default. We can still view the excluded repo with the Repo dropdown by clicking Show All, or by clicking the repo name.

  • We can view only 1 repo by clicking the small only button while hovering a dropdown item
    • This has been applied to the Author dropdown as well
  • Show Default shows all of the repos that aren't

very small deploy_block 🦖 - When you click Show All, the dropdown indicates that only one repo has been selected, instead of all repos. Then if you unselect a repo, and reselect it, the dropdown now indicates that 17 repos have been selected.

Maybe when Show All is selected, we can just show the number of repos that are selected (right now it's 17), and when Show Default, we can hide the number like we currently do?

@kthaler kthaler removed the QAing Under QA team review label Jan 16, 2025
@danielbeardsley
Copy link
Member Author

danielbeardsley commented Jan 16, 2025

When you click Show All, the dropdown indicates that only one repo has been selected, instead of all repos. Then if you unselect a repo, and reselect it, the dropdown now indicates that 17 repos have been selected.

Ah, I see, this is what you meant, the (1) part:
image

We can easily create whatever behavior we come up with. I think "Show Default" should make it look like this:
image

What should "Show All" make it look like? To be clear, show all is still a special state that means "hide nothing" not "Only show these 17 selected things". We could render it like Repo (All) or Repo (17) or it could look exactly like the default mode: Repo.

Thoughts?

@kthaler
Copy link
Contributor

kthaler commented Jan 16, 2025

I think we should keep it simple, and do Repo (17) when you click "Select All" and Repo when you click Select Default. I don't think most devs will ever use "Select All".

I'm open to Repo (All) as well with "Select All", but don't think that's really necessary.

jarstelfox
jarstelfox previously approved these changes Jan 16, 2025
Copy link
Contributor

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

Previously, it would show `{FilterName} (1)` when "Show All" was chosen
because it was reporting the number of values in the url.

Now, it shows the total count any time the selection deviates from the
default.
@danielbeardsley
Copy link
Member Author

I think we should keep it simple, and do Repo (17) when you click "Select All" and Repo when you click Select Default

Done!

image

@danielbeardsley danielbeardsley merged commit ce5883e into master Jan 16, 2025
1 check passed
@danielbeardsley danielbeardsley deleted the hide-repos branch January 16, 2025 23:17
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.

Repos: allow config to exclude certain repos

5 participants