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

Refactor tailer instance selection dropdown #1586

Merged
merged 6 commits into from Jul 14, 2017
Merged

Conversation

kwm4385
Copy link
Contributor

@kwm4385 kwm4385 commented Jul 11, 2017

The instance select dropdown has been refactored into a class and to use react-bootstrap components.

This also fixes two issues:

  • The dropdown will not close when selecting or deselecting an instance, now multiple can be checked without reopening the dropdown.
  • A Select All option has been added which will either select all instances or deselect all but the first instance (at least one instance always needs to be selected or the tailer UI will break).

Caveats:

  • The dropdown will still close when transitioning between one instance selected and greater than one. This is because the header needs to re-render with the appropriate buttons for tailing a single instance.

@kwm4385 kwm4385 added the UI label Jul 11, 2017
@kwm4385 kwm4385 self-assigned this Jul 11, 2017
@kwm4385 kwm4385 requested a review from ssalinas July 11, 2017 17:50
@ssalinas
Copy link
Member

Nice, looks pretty smooth in staging. Isn't an easy way to get around the re-render when going to/from 1 instance. Not worried about that one for now.

Only question is around the limit on selecting. I think the tailer limits to no more than six instances tailed at once, are we enforcing that in any way when clicking select all? (i.e. selects only the first 6 or something?)

@kwm4385
Copy link
Contributor Author

kwm4385 commented Jul 12, 2017

There doesn't appear to be anything stopping me from tailing more than 6 instances in the current implementation. I know it's not very useful from a readability/performance standpoint though.

However it could be more confusing on the user's end to get capped at a seemingly arbitrary number (think about those ultrawide early adopters!). Also if all Select All does is toggle the first six instances I think it loses much of it's usefulness.

@ssalinas
Copy link
Member

Fair enough on usability. Mostly don't want to crash anyones browser from accidentally trying to tail 30 instances or something.

@kwm4385
Copy link
Contributor Author

kwm4385 commented Jul 12, 2017

True. How about we take a middle ground and cap it at the first 8 instances when selecting all? I'm able to open that many without any immediate performance issues in my browser.

@ssalinas
Copy link
Member

Sounds like a plan 👍

@kwm4385 kwm4385 added the hs_qa label Jul 12, 2017
@ssalinas ssalinas modified the milestone: 0.17.0 Jul 14, 2017
@ssalinas ssalinas merged commit 9da6b0f into master Jul 14, 2017
@ssalinas ssalinas deleted the tailer-selectall branch July 14, 2017 19:40
@baconmania baconmania modified the milestones: 0.18.0, 0.17.0 Sep 20, 2017
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.

None yet

3 participants