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: allow long table rows to wrap #29206

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

couchand
Copy link
Contributor

Fixes: #29132

Release note (admin ui change): Allow long table rows to wrap, if necessary.

Before:
screen shot 2018-08-27 at 12 19 04 pm

After:
screen shot 2018-08-28 at 3 30 49 pm

@couchand couchand requested a review from a team August 28, 2018 19:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@mrtracy mrtracy left a comment

Choose a reason for hiding this comment

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

Any chance of this having unintended consequences? I agree that it's not a property you want on your base table, but have we already depended on it elsewhere?

I'm gonna go ahead and approve it, just be on guard in the next few days in case something gets screwed up on one of our other tables.

Copy link
Contributor

@vilterp vilterp left a comment

Choose a reason for hiding this comment

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

Tried it locally and didn't see any issues. It did change the wrapping behavior of the node list page: usually the "address" column wraps and the "version" column doesn't; with this PR it's switched. No worse, no better…

@couchand
Copy link
Contributor Author

couchand commented Sep 4, 2018

Thanks to both of you for being on the lookout for other ramifications.

@vilterp I suspect that the version not wrapping is simply an artifact of changing the available space due to the address wrapping change, but we'd need to dig into it to be more specific. In any case, since the version column in released versions is quite short, this seems like a reasonable tradeoff.

bors r+

@couchand
Copy link
Contributor Author

couchand commented Sep 4, 2018

bors r-

@craig
Copy link
Contributor

craig bot commented Sep 4, 2018

Canceled

Fixes: cockroachdb#29132
Release note (admin ui change): Allow long table rows to wrap,
if necessary.
@couchand couchand force-pushed the fix/wrap-long-lines branch from b2f38ad to a004ddb Compare September 4, 2018 18:53
@couchand
Copy link
Contributor Author

couchand commented Sep 4, 2018

bors r+

craig bot pushed a commit that referenced this pull request Sep 4, 2018
29206: ui: allow long table rows to wrap r=couchand a=couchand

Fixes: #29132

Release note (admin ui change): Allow long table rows to wrap, if necessary.

Before:
<img width="1351" alt="screen shot 2018-08-27 at 12 19 04 pm" src="https://user-images.githubusercontent.com/7085343/44674574-8e0a8000-a9f3-11e8-8b2a-72358f65c098.png">

After:
<img width="1159" alt="screen shot 2018-08-28 at 3 30 49 pm" src="https://user-images.githubusercontent.com/793969/44746086-67be1080-aad7-11e8-8616-158de5354fe1.png">


29402: changefeedccl,jobspb: select initial scan timestamp on gateway r=mrtracy a=danhhz

Fixes a buglet introduced in #28984 where each node in a distsql
changefeed flow would pick its own timestamp to do the initial scan at.

I thought I got all the backward-incompatible changes out of the way
with #27996 but I didn't forsee this one. While I'm making a
backward-incompatible change, switch targets from a `id->string` map to
an `id->proto` map for future expansion. Row and partition watches, for
example, will need this.

Release note (backward-incompatible change): CHANGEFEEDs created with
previous betas and alphas will not work with this version.

Co-authored-by: Andrew Couch <[email protected]>
Co-authored-by: Daniel Harrison <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 4, 2018

Build succeeded

@craig craig bot merged commit a004ddb into cockroachdb:master Sep 4, 2018
@vilterp
Copy link
Contributor

vilterp commented Sep 6, 2018

Just noticed one: made the logs a bit harder to read since the time zones wrap.

image

Sigh. Probably just best to add a nowrap to that column.

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