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

Use flex positioning for card headings #479

Merged
merged 3 commits into from
Sep 6, 2017
Merged

Conversation

chihuahua
Copy link
Member

@chihuahua chihuahua commented Sep 5, 2017

In comments within #430, @wchargin proposed that we use flex positioning
to position elements within the header. That seems like a robust idea.

This PR thus uses @wchargin's proposed method to implement @jart's
original, space-efficient design that makes use of both the left and
right sides of headings (if space is available).

wdyssdns5d1

Having that idea lingering at the end of a closed PR seemed a bit precipitous, so I just went ahead and implemented.

This PR fixes the layout issues brought up in #430. This change also fixes #421.

In comments within #430, @wchargin proposed that we use flex positioning
to position elements within the header. That seems like a robust idea.

This PR thus uses @wchargin's proposed method to implement @jart's
original, space-efficient design of that makes use of both the left and
right sides of headings (if space is available).
@wchargin
Copy link
Contributor

wchargin commented Sep 5, 2017

Cool, this looks nice to me.

(Why is this tagged plugin:images and plugin:audio? Shouldn't it be core:frontend?)

@wchargin
Copy link
Contributor

wchargin commented Sep 5, 2017

I'd prefer if you could note in the commit description why this change was made—i.e., explicitly write (inline in the commit message) the bug that it fixes. (Reason—people abiding by the Chesterton's fence principle might unsoundly revert this if they infer, from the current commit message, that this is just an implementation change without user-facing changes.)

@chihuahua
Copy link
Member Author

Thanks! I like that principle. It's easy for a fence to be removed if its cause for being there isn't clear. How about now?

My latest commit changes a few things to accommodate either (or both) long run names or long tag names.

We use flex-wrap: wrap; to force flex children to wrap to the next line.

download 1
download
pbljkpnf8wn

@wchargin
Copy link
Contributor

wchargin commented Sep 6, 2017

(Right: my original example contained flex-wrap: wrap for this reason. But you're not missing anything else :-) )

Fixing #421 here is fine with me.

@chihuahua chihuahua merged commit c5a16f5 into master Sep 6, 2017
@chihuahua chihuahua deleted the chihuahua-header branch September 6, 2017 00:40
jart pushed a commit to jart/tensorboard that referenced this pull request Sep 6, 2017
In comments within tensorflow#430, @wchargin proposed that we use flex positioning
to position elements within the header. That seems like a robust idea.

This PR thus uses @wchargin's proposed method to implement @jart's
original, space-efficient design of that makes use of both the left and
right sides of headings (if space is available).

This PR fixes the layout issues brought up in tensorflow#430. This change also fixes
tensorflow#421.
@jart
Copy link
Contributor

jart commented Sep 6, 2017

I'm late to this PR but I just want to say that you really managed to perfect what I was going after. I'm very happy with this change.

jart pushed a commit that referenced this pull request Sep 6, 2017
In comments within #430, @wchargin proposed that we use flex positioning
to position elements within the header. That seems like a robust idea.

This PR thus uses @wchargin's proposed method to implement @jart's
original, space-efficient design of that makes use of both the left and
right sides of headings (if space is available).

This PR fixes the layout issues brought up in #430. This change also fixes
#421.
@wchargin
Copy link
Contributor

wchargin commented Sep 6, 2017

Cool, glad to hear it. Thanks!

@chihuahua
Copy link
Member Author

Thanks! :)

@ro3c
Copy link

ro3c commented Sep 13, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

Long event names don't wrap in Tensorboard 61
4 participants