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

report(topbar): fix overflowing url #9497

Merged
merged 1 commit into from Aug 6, 2019

Conversation

psybuzz
Copy link
Contributor

@psybuzz psybuzz commented Aug 1, 2019

This modifies UI in the topbar to

  • Show a tooltip when the URL is hovered
  • Truncate the URL from the end via CSS, based on available space
  • Round Lighthouse icon in the topbar used to shrink due to flexbox, and disappear. Now, it doesn't flex so it should maintain its size.

Screenshot: https://imgur.com/a/2DQ5RmV

Issue: #9142
Associated Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=971585

@psybuzz
Copy link
Contributor Author

psybuzz commented Aug 1, 2019

Hello Lighthouse reviewers! I heard there might be a way to truncate from the middle, but I couldn't find the utility. The screenshot above shows truncation from the right, which I think looks okay. What do others think?

@psybuzz psybuzz marked this pull request as ready for review August 1, 2019 21:51
@psybuzz psybuzz requested a review from paulirish as a code owner August 1, 2019 21:51
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM thanks very much @psybuzz!

@connorjclark was this the method you had in mind for fixing it?

@connorjclark
Copy link
Collaborator

yup this is perfect.

@connorjclark connorjclark changed the title report(topbar): fix styles when narrow report(topbar): fix overflowing url Aug 5, 2019
@psybuzz
Copy link
Contributor Author

psybuzz commented Aug 5, 2019

Thanks for the reviews! I'm not authorized to merge this PR, so feel free to merge it at will.

@patrickhulce
Copy link
Collaborator

It is line break/word wrap related so I feel like we at least have to give @paulirish a chance if he wants since it's almost as if those CSS properties have declared war on us 😆

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking this, though has someone double checked how this works/looks in DevTools, where the bug originated?

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

I'll pile onto the LGTM 🚂

@connorjclark
Copy link
Collaborator

connorjclark commented Aug 6, 2019

LGTM, thanks for taking this, though has someone double checked how this works/looks in DevTools, where the bug originated?

Perhaps @psybuzz could try that out. I could too, would just take awhile to build Chromium away from my desktop.

@psybuzz: basically you just build everything (yarn && yarn build-devtools) then roll to Chromium folder (yarn devtools). If the folder is in the standard location, it'll work. Otherwise, see roll-to-devtools.sh and symlink the folder it's trying to use. Then you can see how it looks in DevTools.

Alternatively, these changes seem benign enough to assume they'll work in DevTools (pending "jinx"...)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

iz good.

@paulirish paulirish merged commit a53fb14 into GoogleChrome:master Aug 6, 2019
@psybuzz
Copy link
Contributor Author

psybuzz commented Aug 7, 2019

Thanks everyone!

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

Successfully merging this pull request may close these issues.

None yet

7 participants