Truncate branch name in prebuilds tab, and more #6907

trumbitta · 2021-11-25T17:03:59Z

Description

Screen.Recording.2021-11-25.at.17.54.17.mov

This does a couple things:

Fixes Truncate branch name on the prebuilds page #5332 by truncating the branch name in the prebuilds tab
Adds a title to both the branch name and the commit (which was already truncated) so that the user is still able to read the complete info even when truncated. See the attached short video.
Aligns the branch name vertically to the rest of the info. Something that was bugging me to no end because I'm both border-line OCD and 45% UX designer 🤪. It's done with retrocompatibility in mind, to avoid a refactor. I'm willing to do the refactor if you want me to, tho.
Updates the README with some info from the awesome @iQQBot , and a screenshot.

Related Issue(s)

How to test

Just start the dashboard and navigate to the prebuild tab of a project. Make sure to have the prebuild of a branch with a long name so that the actual truncation can happen.

Release Notes

Tweaks to the UI of the Prebuilds tab in the dashboard
Some improvements to the README of the dashboard, aimed at contributors

Documentation

iQQBot · 2021-11-25T17:12:54Z

Thank you for this contribution. Before we can merge this PR, we need to make sure we have a Contributor License Agreement (CLA) with you on record. @meysholdt

And please squash these commit into one @trumbitta

iQQBot · 2021-11-25T17:15:01Z

/werft run

👍 started the job as gitpod-build-trumbitta-truncate-branch-name-5332-fork.0

iQQBot · 2021-11-25T17:37:24Z

And you maybe need change README.png to another name eg how-to-get-cookie.png, because README.png seem not pass pre-commit check

trumbitta · 2021-11-25T19:28:40Z

@iQQBot sure thing, doing it all right now!

Do you need me to do something about the CLA or should I wait for input by @meysholdt ?
I chose the name README.png to keep it well sorted near the README.md :-/ is there a way for me to run the pre-commit hook before submitting the update, so that I can find the best name?
[UPDATE]: I tried pre-commit run --all-files which I found in werft logs, but it fails with an error about --user not possible in a virtualenv, so I think I'll just keep the name you suggested 😺
Are you sure you want me to squash all of the commits into one? What if I squash together the 3 commits that update the prebuilds tab, then keep the one for the ItemField component and the one for the README apart? 3 commits total after I rebase them. (I'll do it like this without waiting for your reply, since I can always squash them all into one if you say "no, squash them all" 😄 )

gtsiolis · 2021-11-26T00:44:34Z

/werft run

👍 started the job as gitpod-build-trumbitta-truncate-branch-name-5332-fork.1

trumbitta · 2021-11-26T11:01:00Z

Oof I fat-fingered a merge from main into this branch. Going to delete the commit right now.

trumbitta · 2021-11-26T11:08:17Z

No ok, I'm risking of doing more harm and it should be harmless to leave it since it's all going to go back on main either way

👀

meysholdt · 2021-11-26T15:17:35Z

hi @trumbitta ! I've send you a CLA to sign via DocuSign. If you have questions, feel free to ping me via moritz@gitpod.io.

trumbitta · 2021-11-26T15:27:45Z

hi @trumbitta ! I've send you a CLA to sign via DocuSign. If you have questions, feel free to ping me via moritz@gitpod.io.

Done! 🖊️

meysholdt · 2021-11-26T15:56:02Z

Thank you @trumbitta for signing the CLA!

jankeromnes

Hi @trumbitta, many thanks for improving Gitpod's dashboard! ✨ 🙏

No ok, I'm risking of doing more harm and it should be harmless to leave it since it's all going to go back on main either way

No worries here. We want to avoid merge commits on Gitpod's main branch, but since this Pull Request will get "Rebase&Merge"d eventually, this will remove the merge commit automatically.

By the way, if you wanted to remove the merge commit yourself (no need to, but just in case), you could:

Open this Pull Request in Gitpod: https://gitpod.io/#https://github.com/gitpod-io/gitpod/pull/6907
Run git pull --rebase upstream main (this will rebase your commits and remove the merge commit)
Run git push -f origin trumbitta/truncate-branch-name-5332 (the force-push is needed in order to change or remove commits from your branch)

I've had a quick look at your changes, and I must say I really like the README.md clarifications, and the use of truncate classes and title attributes. 💯 I would love to see them merged.

One minor point that is causing me to delay the approval of this Pull Request is the new shouldCenterVertically prop, and the slight ambiguity it creates on the CSS classes. Please see my comment inline, and let me know what you think of it (please address it if you agree, but it's also fine to simply reject my comment if you disagree).

jankeromnes · 2021-11-27T08:41:45Z

components/dashboard/src/components/ItemsList.tsx

-        {props.children}
-    </div>;
+  return (
+    <div className={`flex-grow mx-1 ${props.className || ""} ${props.shouldCenterVertically ? "my-auto" : "my-0"}`}>


Thank you for making the vertical centering optional!

However, I see that you can now set vertical margins via both props.className (e.g. by including my-auto and/or my-0 in the classes) or the new props.shouldCenterVertically boolean. This creates a slight ambiguity, as future developers might accidentally provide conflicting margin classes, and then wonder why their vertical centering doesn't work properly.

How about shifting the responsibility of my-auto vs my-0 (or no my class at all) to the users of ItemField instead? (I.e. remove my-auto from the className here as you did, but don't introduce a props.shouldCenterVertically -- simply let all the users of ItemField provide a my-auto via the props.className if needed).

Please let me know what you think of this alternative proposal. 🙂

100% agree!

I don't know if you had the chance to take a look at my commit messages, but the gist of it is I chose to add a prop purposely for limiting the scope of my intervention and "don't go too far my first time around".

I'm totally up for a refactoring involving the 5-6 components where this one is used and removing the new prop, and I'll work on it as soon as I am able.

As a side note, there's also something that makes my spider-senses tingle about having to use both my-auto and items-center to center things, but at this moment I just don't know Tailwind nor how it's used in this codebase well enough to propose anything about it.

Refactoring done!

Refactoring done!

Wow, many thanks! 💯 🙏 I'll take another look at your changes today.

As a side note, there's also something that makes my spider-senses tingle about having to use both my-auto and items-center to center things, but at this moment I just don't know Tailwind nor how it's used in this codebase well enough to propose anything about it.

You're absolutely right, that's an excellent point. 🎯 From my perspective:

centering things with flex is far superior to centering with margins (flex makes it quite easy, and the produced layout "makes sense" and works well in the majority of situations regardless of screen size, doesn't create awkward overflows, etc.)

there is still a lot of margin-centering in the code base (or, worse, both margin- and flex-centering combined 🙈) which could be improved by removing all the auto margins and using flex instead

However, in the interest of not derailing this Pull Request too much, maybe let's focus on the my-auto refactoring here. We can always switch to flex in future Pull Requests. 😁

jankeromnes · 2021-11-27T08:42:30Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -193,13 +193,13 @@ export default function () {
                    </ItemField>
                    <ItemField className="flex items-center">
                        <div className="truncate">
-                            <div className="text-base text-gray-500 dark:text-gray-50 font-medium mb-1 truncate">{shortCommitMessage(p.info.changeTitle)}</div>
+                            <div className="text-base text-gray-500 dark:text-gray-50 font-medium mb-1 truncate" title={shortCommitMessage(p.info.changeTitle)}>{shortCommitMessage(p.info.changeTitle)}</div>


Yes 🙏 thank you for that!

jankeromnes · 2021-11-27T08:45:08Z

components/dashboard/src/projects/Prebuilds.tsx

-                            <span className="font-medium text-gray-500 dark:text-gray-50">{p.info.branch}</span>
+                    <ItemField className="flex" shouldCenterVertically={false}>
+                        <div className="flex space-x-2 truncate">
+                            <span className="font-medium text-gray-500 dark:text-gray-50 truncate" title={p.info.branch}>{p.info.branch}</span>


Also looks good to me!

One small nit: I think it's an anti-pattern to add a truncate class to both an element and its container. In theory, only one should be required. Maybe you can remove the truncate class from the container? (Haven't actually tested this -- if it doesn't work, please don't worry about this comment 🙏)

Again, 100% agree.

When I was starting to work on this, I think I found an old commit adding the same thing to another piece of the app (maybe it was just a few lines above these, I can't remember) and I found the double truncate to be weird... but it turns out it is needed! It doesn't work without.

That's because the CSS ellipsis needs the parent to have certain properties in order to work as expected, and the truncate class happens to also add those. So at the end of the day it's a hack, a workaround, that I found in the codebase and applied here.

Here's my proposal: I'll create a new TruncatedText component in components/dashboard/src/components which will:

provide the necessary CSS "environment" for truncate to work

use truncate on its children text

accept a title prop

This will result in another refactoring over 38 files 😅 but, hey, I'll do it 💪.

Thoughts?
Also, since this new double truncate is perfectly in line with what's already around there, maybe I could open another issue / PR and work on it as my next task?

Aha, many thanks for explaining why the double truncate is needed @trumbitta.

Maybe the underlying reason is that Tailwind's truncate class is actually a combination of several CSS classes: https://tailwindcss.com/docs/text-overflow

If possible, I think I'd prefer a refactoring that makes our code always use truncate class(es) properly, instead of creating a brand new React component to solve this, but I could be wrong here.

Also, since this new double truncate is perfectly in line with what's already around there, maybe I could open another issue / PR and work on it as my next task?

Agreed. 👍 I think you could indeed open an issue about unifying how we truncate text, and we could move this discussion there. Thanks for caring about this!

iQQBot · 2021-11-27T20:27:08Z

/werft run

👍 started the job as gitpod-build-trumbitta-truncate-branch-name-5332-fork.2

1. Truncate branch name in prebuilds tab 2. Add long form titles to truncated info in prebuilds tab 3. In the prebuilds tab, vertically align the branch name with the other info

jankeromnes · 2021-11-29T10:15:06Z

/werft run

👍 started the job as gitpod-build-trumbitta-truncate-branch-name-5332-fork.3

jankeromnes

Code looks good to me, and works like a charm. ✨ Many thanks again for improving the dashboard! 💯

I've noticed a few places where the margin-centering could be replaced by flex-centering, but this Pull Request is already a net positive as is, so I'd love to see it merged. 🚀

If you're interested and have time, I'd be more than happy to discuss / help with / review follow-up Pull Requests. 😁

/approve
/lgtm

jankeromnes · 2021-11-29T10:12:50Z

components/dashboard/src/projects/Prebuilds.tsx

                        <span>Branch</span>
                    </ItemField>
                </Item>
                {prebuilds.filter(filter).sort(prebuildSorter).map((p, index) => <Item key={`prebuild-${p.info.id}`} className="grid grid-cols-3">
-                    <ItemField className="flex items-center">
+                    <ItemField className="flex items-center my-auto">


Aha, I think here the my-auto isn't actually needed (since flex items-center already does the vertical centering).

Suggested change

<ItemField className="flex items-center my-auto">

<ItemField className="flex items-center">

Yeah, I just went for a safe refactoring: "monkey pulls my-auto out of ItemField component, monkey adds my-auto to ItemField classes everywhere" 😄

That's the best kind of refactoring 😁 🙏

jankeromnes · 2021-11-29T10:13:11Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -191,15 +191,15 @@ export default function () {
                            <p>{p.info.startedByAvatar && <img className="rounded-full w-4 h-4 inline-block align-text-bottom mr-2" src={p.info.startedByAvatar || ''} alt={p.info.startedBy} />}Triggered {formatDate(p.info.startedAt)}</p>
                        </Link>
                    </ItemField>
-                    <ItemField className="flex items-center">
+                    <ItemField className="flex items-center my-auto">


Same here:

Suggested change

<ItemField className="flex items-center my-auto">

<ItemField className="flex items-center">

jankeromnes · 2021-11-29T10:13:53Z

components/dashboard/src/projects/Project.tsx

@@ -228,21 +228,21 @@ export default function () {
                        const status = prebuildStatusLabel(prebuild);

                        return <Item key={`branch-${index}-${branch.name}`} className="grid grid-cols-3 group">
-                            <ItemField className="flex items-center">
+                            <ItemField className="flex items-center my-auto">


Same:

Suggested change

<ItemField className="flex items-center my-auto">

<ItemField className="flex items-center">

jankeromnes · 2021-11-29T10:14:02Z

components/dashboard/src/projects/Project.tsx

                                <div>
                                    <a href={branch.url}><div className="text-base text-gray-600 hover:text-gray-800 dark:text-gray-50 dark:hover:text-gray-200 font-medium mb-1">
                                        {branch.name}
                                        {branch.isDefault && (<span className="ml-2 self-center rounded-xl py-0.5 px-2 text-sm bg-blue-50 text-blue-40 dark:bg-blue-500 dark:text-blue-100">DEFAULT</span>)}
                                    </div></a>
                                </div>
                            </ItemField>
-                            <ItemField className="flex items-center">
+                            <ItemField className="flex items-center my-auto">


Same:

Suggested change

<ItemField className="flex items-center my-auto">

<ItemField className="flex items-center">

jankeromnes · 2021-11-29T10:14:10Z

components/dashboard/src/projects/Project.tsx

                                <div className="truncate">
                                    <div className="text-base text-gray-500 dark:text-gray-50 font-medium mb-1 truncate">{shortCommitMessage(branch.changeTitle)}</div>
                                    <p>{avatar}Authored {formatDate(branch.changeDate)} · {branch.changeHash?.substring(0, 8)}</p>
                                </div>
                            </ItemField>
-                            <ItemField className="flex items-center">
+                            <ItemField className="flex items-center my-auto">


Same:

Suggested change

<ItemField className="flex items-center my-auto">

<ItemField className="flex items-center">

roboquat · 2021-11-29T10:41:31Z

LGTM label has been added.

Git tree hash: 6991a620a0fc6ea32cc075f2f6f8e679eb42cfb5

roboquat · 2021-11-29T10:41:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jankeromnes

Associated issue: #5332

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/dashboard/OWNERS~~ [jankeromnes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

roboquat added release-note team: webapp Issue belongs to the WebApp team size/M labels Nov 25, 2021

iQQBot added the community-contribution label Nov 25, 2021

gtsiolis added the do-not-merge/cla-pending CLA has not been signed label Nov 25, 2021

trumbitta force-pushed the trumbitta/truncate-branch-name-5332 branch from 4103e71 to ccdf749 Compare November 25, 2021 19:51

gtsiolis mentioned this pull request Nov 25, 2021

Add labels for CLA signing status on community contributions gitpod-io/gitbot#53

Closed

meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Nov 26, 2021

jankeromnes reviewed Nov 27, 2021

View reviewed changes

trumbitta force-pushed the trumbitta/truncate-branch-name-5332 branch from 5e85f26 to afadeba Compare November 28, 2021 21:15

trumbitta added 3 commits November 28, 2021 22:35

[dashboard] make ItemField agnostic about vertical centering

8e473eb

[dashboard] tweak UI in prebuilds tab

37d33eb

1. Truncate branch name in prebuilds tab 2. Add long form titles to truncated info in prebuilds tab 3. In the prebuilds tab, vertically align the branch name with the other info

[docs]: clarify how to configure the dashboard for contributing

8720b99

trumbitta force-pushed the trumbitta/truncate-branch-name-5332 branch from afadeba to 8720b99 Compare November 28, 2021 22:53

roboquat added size/L and removed size/M labels Nov 28, 2021

jankeromnes approved these changes Nov 29, 2021

View reviewed changes

roboquat assigned jankeromnes Nov 29, 2021

roboquat added the lgtm label Nov 29, 2021

roboquat added the approved label Nov 29, 2021

roboquat merged commit e2c1988 into gitpod-io:main Nov 29, 2021

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncate branch name in prebuilds tab, and more #6907

Truncate branch name in prebuilds tab, and more #6907

trumbitta commented Nov 25, 2021

iQQBot commented Nov 25, 2021

iQQBot commented Nov 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

iQQBot commented Nov 25, 2021

trumbitta commented Nov 25, 2021 •

edited

Loading

gtsiolis commented Nov 26, 2021 •

edited by werft-gitpod-dev-com bot

Loading

trumbitta commented Nov 26, 2021

trumbitta commented Nov 26, 2021 •

edited

Loading

meysholdt commented Nov 26, 2021

trumbitta commented Nov 26, 2021 •

edited by werft-gitpod-dev-com bot

Loading

meysholdt commented Nov 26, 2021

jankeromnes left a comment •

edited

Loading

jankeromnes Nov 27, 2021 •

edited

Loading

trumbitta Nov 27, 2021

trumbitta Nov 28, 2021

jankeromnes Nov 29, 2021

jankeromnes Nov 27, 2021

jankeromnes Nov 27, 2021

trumbitta Nov 27, 2021

jankeromnes Nov 29, 2021

iQQBot commented Nov 27, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Nov 29, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes left a comment

jankeromnes Nov 29, 2021

trumbitta Nov 29, 2021

jankeromnes Nov 29, 2021

jankeromnes Nov 29, 2021

jankeromnes Nov 29, 2021

jankeromnes Nov 29, 2021

jankeromnes Nov 29, 2021

roboquat commented Nov 29, 2021

roboquat commented Nov 29, 2021

	<ItemField className="flex items-center my-auto">
	<ItemField className="flex items-center">

Truncate branch name in prebuilds tab, and more #6907

Truncate branch name in prebuilds tab, and more #6907

Conversation

trumbitta commented Nov 25, 2021

Description

Related Issue(s)

How to test

Release Notes

Documentation

iQQBot commented Nov 25, 2021

iQQBot commented Nov 25, 2021 • edited by werft-gitpod-dev-com bot Loading

iQQBot commented Nov 25, 2021

trumbitta commented Nov 25, 2021 • edited Loading

gtsiolis commented Nov 26, 2021 • edited by werft-gitpod-dev-com bot Loading

trumbitta commented Nov 26, 2021

trumbitta commented Nov 26, 2021 • edited Loading

meysholdt commented Nov 26, 2021

trumbitta commented Nov 26, 2021 • edited by werft-gitpod-dev-com bot Loading

meysholdt commented Nov 26, 2021

jankeromnes left a comment • edited Loading

Choose a reason for hiding this comment

jankeromnes Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iQQBot commented Nov 27, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes commented Nov 29, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roboquat commented Nov 29, 2021

roboquat commented Nov 29, 2021

iQQBot commented Nov 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

trumbitta commented Nov 25, 2021 •

edited

Loading

gtsiolis commented Nov 26, 2021 •

edited by werft-gitpod-dev-com bot

Loading

trumbitta commented Nov 26, 2021 •

edited

Loading

trumbitta commented Nov 26, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes left a comment •

edited

Loading

jankeromnes Nov 27, 2021 •

edited

Loading

iQQBot commented Nov 27, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Nov 29, 2021 •

edited by werft-gitpod-dev-com bot

Loading