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

chore(dashboard): adjust prebuild table col width #10564

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

andreafalzetti
Copy link
Contributor

Description

The Prebuilds page not always align correctly the columns, e.g. see screeenshot below:

Screenshot 2022-06-09 at 11 21 36 copy

I think this small CSS change could improve already some scenarios.

Related Issue(s)

N/A

How to test

  1. Start the dashboard for gitpod.io

(I would also suggest Prev Env, but there are no prebuilds there)

Before After
Screenshot 2022-06-09 at 11 21 36 Screenshot 2022-06-09 at 11 21 03

Release Notes

NONE

@andreafalzetti andreafalzetti force-pushed the afalz/webapp-prebuilds-col-width branch from e463eca to 388098a Compare June 9, 2022 16:40
@geropl
Copy link
Member

geropl commented Jun 13, 2022

@AlexTugarev Could you review this PR (tomorrow)? 🙏

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 14, 2022

Happy to take a look 👀

@jankeromnes jankeromnes self-assigned this Jun 14, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 14, 2022

/werft run

👍 started the job as gitpod-build-afalz-webapp-prebuilds-col-width.3
(with .werft/ from main)

@@ -190,7 +190,7 @@ export default function (props: { project?: Project; isAdminDashboard?: boolean
</div>
<ItemsList className="mt-2">
<Item header={true}>
<ItemField className="my-auto w-5/12">
<ItemField className="my-auto md:w-3/12 xl:w-4/12">
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to not overshoot at md.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tip for the future: Using CSS grid is much more powerful and robust in these situation than trying to guess good fixed widths.

With grid, you can simply specify "I want three columns of exactly the same width", or specify custom relative column widths, but they'll almost always be correct regardless of screen size, margins, size of content, etc.

@AlexTugarev
Copy link
Member

@jankeromnes, are you still looking at this? This seems to fix a bigger issue, and I'm keen to merge it as is. wdyt?

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 14, 2022

Ah, yes, thanks for the reminder. Triggered a build then forgot about this.

Resuming review now. 👀

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving our Prebuilds list! ✨

I tested this change at various resolutions, and with various commit messages / branch name lengths. I think the current implementation is still not perfect (e.g. might need an ellipsis + tooltip on the branch name) hence the hold, but please feel free to merge this when you like (this is easy to adjust later as well).

FYI, here are my test results:

My normal screen size Max screen size Smartphone (>50% of web users) iPad (portrait)
Screenshot 2022-06-14 at 11 25 40 Screenshot 2022-06-14 at 11 26 09 Screenshot 2022-06-14 at 11 26 43 Screenshot 2022-06-14 at 11 27 01

Tip: To automatically add an ellipsis to a potentially super long cell (e.g. the branch name), you can add:

  • the truncate CSS class
  • a title attribute with the exact same text content as the element (so that users can still hover over the truncated text to see the full text)

See also how the commit message is truncated for inspiration.

/hold

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 14, 2022

Oh, wait, I wonder if #10472 maybe already fixes the same issue? 👀

Looks like we may need a overflow-hidden in addition to the truncate class. 💡

@akosyakov
Copy link
Member

should be merged or closed?

@gtsiolis
Copy link
Contributor

I'd suggest getting this merged as it's already a good MVC improvement, regardless if the rendering on small (mobile) viewports looks great, leaving this for #4050.

If that's useful, I'd like to help get #10472 also merged afterwards.

@gtsiolis gtsiolis closed this Jun 27, 2022
@gtsiolis gtsiolis reopened this Jun 27, 2022
@akosyakov
Copy link
Member

@jankeromnes @AlexTugarev is it alright to remove hold?

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 29, 2022

Hi! Yes, and sorry for sitting on this for so long.

@gtsiolis's comment makes a good case or merging this. 👍 Thus, removing the hold.

/unhold

@roboquat roboquat merged commit 1e256ee into main Jun 29, 2022
@roboquat roboquat deleted the afalz/webapp-prebuilds-col-width branch June 29, 2022 13:41
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dashboard deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants