Skip to content

Conversation

@jameswex
Copy link
Contributor

  • Motivation for features / changes

Long WIT error messages caused text overflow, hiding some controls.

  • Technical description of changes

Added correct CSS rules to avoid overflow issue and to ellipsis the status string. The full error string is already shown in a toast as well.

  • Screenshots of UI changes

Fixed:
overflow

  • Detailed steps to verify changes work correctly (as executed by you)

Ran demo app, added fake very long error message, verified correct visual behavior (and incorrect behavior before fix).

@jameswex
Copy link
Contributor Author

@tolga-b please review

Copy link
Contributor

@tolga-b tolga-b left a comment

Choose a reason for hiding this comment

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

LGTM.

@jameswex jameswex requested a review from stephanwlee July 25, 2019 20:06
.datapoint-right-controls-holder {
display: flex;
flex-direction: row-reverse;
overflow: hidden;
Copy link
Contributor

@stephanwlee stephanwlee Jul 25, 2019

Choose a reason for hiding this comment

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

I think it is more correct to put flex-shrink: 0; and remove the min-width.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try that but it doesn't work. With this container having flex-shrink: 0, the child div with lots of text will still cause this container to grow in width beyond the screen, leading to the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to flex-shrink: 0 for the two buttons as you suggested and removed the min-width setting for the button.

still need the overflow: hidden on the right-controls-holder to avoid overflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, understand that now.

@stephanwlee
Copy link
Contributor

Btw, I think the text/buttons should be vertically aligned :)

@jameswex
Copy link
Contributor Author

Btw, I think the text/buttons should be vertically aligned :)

adjusted margin so that status text matches alignment for left-side tab text and right side buttons. Thanks.

white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
padding-top: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think the more robust solution is to use align-items: center on flex container and remove all these padding/margins.

@jameswex jameswex merged commit c00d7fc into tensorflow:master Jul 29, 2019
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.

3 participants