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

Update user-facing terminology in the vtctld2 UI #6481

Merged
merged 4 commits into from
Jul 27, 2020
Merged

Update user-facing terminology in the vtctld2 UI #6481

merged 4 commits into from
Jul 27, 2020

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Jul 27, 2020

...because I'm a little bit cowardly of making logic changes to an unfamiliar UI, I figured it was less risky to only change the display values and leave the Typescript alone (for now). Unfortunately, this leaves a handful of instances of oppressive terminology in the code base, but they won't be user-facing.

The static files and rice-box.go changes were auto-generated with the new build tooling.

One thing worth noting is that this overwrites the files to go/vt/vtctld/rice-box.go that were made in 1acb6e0. Trying to parse that diff is an absolute nightmare, so I was unsuccessful in copying over the changes. 😞 I don't see any updates to the unbuilt web/vtctld2/ files that might suggest what the changes were -- @deepthi do you happen to remember what they were, and if I should copy them over?

I also updated the copyright date in the new FE build scripts per @rohit-nayak-ps's suggestion on #6473. :)

Here's one example of the UI before vs. after:

Screen Shot 2020-07-27 at 1 27 05 PM

Screen Shot 2020-07-27 at 2 52 17 PM

doeg added 2 commits July 27, 2020 14:39
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg doeg requested a review from sougou as a code owner July 27, 2020 19:00
@deepthi
Copy link
Member

deepthi commented Jul 27, 2020

One thing worth noting is that this overwrites the files to go/vt/vtctld/rice-box.go that were made in 1acb6e0. Trying to parse that diff is an absolute nightmare, so I was unsuccessful in copying over the changes. I don't see any updates to the unbuilt web/vtctld2/ files that might suggest what the changes were -- @deepthi do you happen to remember what they were, and if I should copy them over?

I don't remember why I decided to run make embed_static on that PR, but you are correct that there were no actual UI changes. It is possible that there were earlier PRs that had been merged without updating rice-box.go. Since you have a working UI, what you have is now canonical 😄

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Stop Slave was a reference to the actual mysql command and made sense, but it is better to now refer to it as Start/Stop/starting/stopping Replication because you are not stopping or starting the whole instance or process, just the replication stream. This is also what we did on the server side.

web/vtctld2/src/app/dashboard/shard.component.ts Outdated Show resolved Hide resolved
web/vtctld2/src/app/dashboard/shard.component.ts Outdated Show resolved Hide resolved
web/vtctld2/src/app/dashboard/tablet.component.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

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

LGTM.
Agree with Deepthi: any regressions due to people having modified the bundles directly can be fixed "properly" once they are discovered!

doeg added 2 commits July 27, 2020 16:09
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Copy link
Contributor Author

doeg commented Jul 27, 2020

Sorry it took so long to apply your suggestions, @deepthi! I blame Docker + my terrible internet. 😂

Here's what it looks like now:

Screen Shot 2020-07-27 at 4 39 52 PM

Screen Shot 2020-07-27 at 4 39 54 PM

It might be a good idea to squash merge this PR. 🎃 I've been committing the .ts file changes + the generated assets in separate commits to tidy up the commit history; thinking in terms of rollbacks + cherry picking, it probably makes the most sense to treat this PR as an atomic unit. Up to you though!

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@deepthi
Copy link
Member

deepthi commented Jul 27, 2020

All tests are showing green in the Actions view.

@deepthi deepthi merged commit 5bffd6f into vitessio:master Jul 27, 2020
This was referenced Jul 27, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
deepthi added a commit that referenced this pull request Jul 28, 2020
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.

3 participants