search

build-canaries / nevergreen

:baby_chick: A build monitor with attitude
https://nevergreen.io
Eclipse Public License 1.0
215 stars 38 forks source link

includeAll & excludeAll what is visible #243

Closed rafasf closed 6 years ago

rafasf commented 6 years ago

Motivation

We have a shared CI server and own multiple jobs. We felt that was quite challenging to select project by project when filtering for the prefix of our team.

I'm wondering if that is a valid case, given the path of evolution for nevergreen.

The Change

This will change includeAll and excludeAll to perform such action in what is visible in the screen.

Fixes issue (I can create one if the use case is valid)

@build-canaries/owners

GentlemanHal commented 6 years ago

Hi @rafasf, thanks for submitting this PR!

When we initially added the filtering we had a conversation about what to do with the buttons. We came to the conclusion it would be less confusing to just disable them.

However your use case does make sense, so I'd like some more opinions before merging; @joejag @cowley05

Perhaps we could also change the wording on the buttons when filtering is enabled?

e.g. INCLUDE VISIBLE

rafasf commented 6 years ago

Hi @GentlemanHal, changing the text to be more explict of what would happen is a good option. It help existing users by not introducing a new behaviour to the same button.

Though I noticed in a few other applications that "select all" usually acts on whatever is in the screen.

I will wait for other opinions before I make any changes 😄

cowley05 commented 6 years ago

I think that as long as it is clear that the button is acting only on the visible items then that is fine.

It actually does make sense to me if you have filtered and then click "select all" that it would be all the filtered options that get added but changing the text is also fine. (no strong opinion either way)

Thanks for contributing!

rafasf commented 6 years ago

Cool! @GentlemanHal let me know if you prefer changing the text or if the current change is good.

GentlemanHal commented 6 years ago

Hey @rafasf, I've merged this into master. We can always change the wording later if we need too. Sorry it took so long!