Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Add global recoveries button #646

Closed
wants to merge 3 commits into from
Closed

Conversation

mateusduboli
Copy link
Contributor

A Pull Request should be associated with an Issue.

Related issue: #645

Description

Add a button on the interface to show and change the global-recovery status:

Example

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh
  • code is tested via go test ./go/...

@shlomi-noach shlomi-noach self-requested a review October 7, 2018 03:14
@shlomi-noach
Copy link
Collaborator

Thank you for this feature!
I find that the position of the button on the navigation bar consumes too much space. Would you be open to relocate it under, say, the context menu, as other toggles seen in this screenshot?

screen shot 2018-10-07 at 06 15 38

@sjmudd
Copy link
Collaborator

sjmudd commented Oct 8, 2018

Mateus, I agree with Shlomi on the size of the button/display.

I'd suggest:

  • remove text "global" from the text as you're looking at a global dashboard. So just "recoveries enabled" or "recoveries disabled"
  • change the "recoveries disabled" to red as if disabled we want this to be very visible.

Shlomi I'd rather see a smaller icon on the top bar. If recoveries are disabled you really want this to be visible in all views.

My preferred location would be sort of between the orchestrator text and the home button but to have an icon which is small and red (disabled) or green (enabled). The exact type of icon or spare probably doesn't matter that much,I'd favour a single light traffic light icon, and a mouse over would give the full text "Recoveries disabled globally" or "Recoveries enabled globally".

@mateusduboli
Copy link
Contributor Author

Hello thanks for the feedback!

@sjmudd I'm adding "global" to the name as this button is shown in all contexts, it's not aware if either the page is looking at a cluster or looking at all of them.

@shlomi-noach, I was thinking the directions of the "read-only" icon when someone is using orchestrator without modification privileges.

I could think of using a more clear a icon or maybe use a mouse-over text to explain what it means.
(Mouse over the elements)

❤️
💔

@mateusduboli
Copy link
Contributor Author

mateusduboli commented Oct 8, 2018

I did a quick example here, see if it's good

(The mouse hover shows the correct text, but my recording it's now showing it for some reason)

screencast-localhost-3000-2018 10 08-16-16-23

@sjmudd
Copy link
Collaborator

sjmudd commented Oct 9, 2018

The [broken] heart icon is I think confusing but icon size and placement is better.

@shlomi-noach shlomi-noach self-assigned this Oct 10, 2018
@shlomi-noach
Copy link
Collaborator

Heart (full/empty) icon is definitely much better, and consistent with sidebar icons. I'm OK to go with that.

@mateusduboli
Copy link
Contributor Author

I've pushed the code with the heart icon now, and added a alert when the user clicks the button to add some security, since this can be a very dangerous action.

@shlomi-noach
Copy link
Collaborator

In the future, and to reduce friction, please submit a PR not from your master but from a branch. It just makes the testing more trivial. Thanks!

@shlomi-noach shlomi-noach temporarily deployed to production/mysql_cluster=concertmaster October 11, 2018 11:18 Inactive
@shlomi-noach
Copy link
Collaborator

#654 resubmits this PR and is merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants