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(tables): change table layout to fixed to ensure proper display… #8514

Closed
wants to merge 1 commit into from

Conversation

ahmadalfy
Copy link

Change table layout to fixed to ensure proper display of all columns

Summary
This is a fix for layout bug in the generated report. Currently when the number of characters in the URLs exceed the allowed space it results in a display that looks like this:

image

As you can see, the information for "Potential Savings (KB)" is pushed and cannot be displayed. Fixing that by changing the table layout to fixed fix the issue.

image

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.

Thanks for the contribution @ahmadalfy!

Tried on the new report CSS too, and seems to fix the long URL issue quite nicely :)

Before
image

After
image

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.

LGTM.

We do still do this kind of deal on small screens though afaict:
image

@ahmadalfy
Copy link
Author

Thank you, sorry if I merged to master directly should I create a branch and resubmit the PR? I didn't find that in the contribution guide.

@brendankenny
Copy link
Member

I know @paulirish has looked at the table layouts before...

@paulirish paulirish self-assigned this Apr 22, 2019
@brendankenny
Copy link
Member

we're going to wait on this until after the rest of #8185 lands to make table changes

@paulirish
Copy link
Member

@ahmadalfy thx very much for writing this. I ended up going a bit deeper and sorting this out in #8820

@paulirish paulirish closed this May 3, 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.

5 participants