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

ui: a variety of minor fixes #23904

Merged
merged 6 commits into from
Mar 20, 2018
Merged

Conversation

couchand
Copy link
Contributor

@couchand couchand commented Mar 15, 2018

@couchand couchand requested a review from a team March 15, 2018 16:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vilterp
Copy link
Contributor

vilterp commented Mar 15, 2018

Thank you for getting to all this stuff!

@vilterp
Copy link
Contributor

vilterp commented Mar 15, 2018

re: removing the backlinks: would be nice to highlight the sidebar icon you're under so you know "where you are" in the UI. It would effectively become your backlink. Note that the icons are currently highlighted when you're on the top-level page itself; maybe we can use the same style. Kind of disorienting otherwise.

re: "custom graph" => "custom chart", may collide with #23771 (race condition!)


Reviewed 5 of 5 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 8 of 8 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 81 at r5 (raw file):

  componentDidUpdate() {
    hljs.highlightBlock(this.createStmtNode);

huh. Verified the fix but don't understand why componentDidMount isn't enough. Oh… tables not loaded yet so the pre is empty? Gah.


pkg/ui/src/views/shared/util/table.styl, line 43 at r3 (raw file):

    border-right $table-cell-border
    font-weight inherit
    vertical-align top

nice. also makes it jitter a little less when you expand… just adds content at the bottom; doesn't move up to the top of the cell since it's already there.


Comments from Reviewable

@couchand
Copy link
Contributor Author

rebased, ready to go, PTAL

The only one that naturally has a "where you are" is the table details page. I've opened #24069 to track updating the highlight logic for that, but since it looks like it will be nontrivial I'm not going to try to address it here.


Review status: 4 of 12 files reviewed at latest revision, 2 unresolved discussions.


pkg/ui/src/views/databases/containers/tableDetails/index.tsx, line 81 at r5 (raw file):

Previously, vilterp (Pete Vilter) wrote…

huh. Verified the fix but don't understand why componentDidMount isn't enough. Oh… tables not loaded yet so the

 is empty? Gah.

yeah..............


pkg/ui/src/views/shared/util/table.styl, line 43 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

nice. also makes it jitter a little less when you expand… just adds content at the bottom; doesn't move up to the top of the cell since it's already there.

yay!


Comments from Reviewable

@vilterp
Copy link
Contributor

vilterp commented Mar 20, 2018

:lgtm: on everything but removing backlinks everywhere. Mind splitting that commit out into its own PR? I just think it needs a little more thought and don't want to block the other good stuff in here.

The case of the cluster overview page and the node details page is tricky, since cluster overview can be in so many states (list vs map; zoomed into a locality). I think I'd be ok with removing the "back to node list" link there. But removing it from tables seems like a usability regression.


Reviewed 11 of 14 files at r6.
Review status: 4 of 16 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@couchand
Copy link
Contributor Author

I think we need to do something to clean up the nodes links for this release, and it seems like removing the link is the best option we have right now. I've updated the back links commit to leave the database table back link, even though I don't like having inconsistency, so that it's less contentious here.

@vilterp
Copy link
Contributor

vilterp commented Mar 20, 2018

Don't love the inconsistency either, but I think it's the right compromise for now. :shipit:

The explicit back links are hard to keep up, particularly since a given page
may be linked to from multiple locations.  Since the browser back button works
just as well, this removes the back links from nodes & events pages.

Fixes: cockroachdb#23682
Release note (admin ui change): Remove explicit back links on Events and Nodes
pages.
This makes the top bar styles on the node list match those of the node map.

Fixes: cockroachdb#23802
Release note: None
This fixes a rare issue where a valid timestamp cannot be found for a
particular hover point on the time series charts.  For now it ignores a related
set of problems where time series have different missing points.

Fixes: cockroachdb#23011
Release note: None
@couchand couchand merged commit 5d6d361 into cockroachdb:master Mar 20, 2018
@couchand couchand deleted the fix/ui-tweaks branch March 20, 2018 22:10
@couchand
Copy link
Contributor Author

I'm planning on cherry-picking this into 2.0.1, unless @bdarnell thinks it's worth bringing over now. Perhaps just c5527d0, since that's small and relatively obvious/annoying?

@bdarnell
Copy link
Contributor

I think the rename customgraph to customchart is worth getting in to 2.0 so we don't immediately break any bookmarks people may create. I'm OK with cherry-picking this whole batch of changes as long as you're confident in them.

@vilterp
Copy link
Contributor

vilterp commented Mar 21, 2018

I've tried out these changes locally and am pretty confident they're fine to go into 2.0.

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.

4 participants