[dashboard] Fix feedback when triggering prebuilds #5731

jankeromnes · 2021-09-17T07:13:35Z

Description

Related Issue(s)

How to test

Add a Project
Trigger a Prebuild by clicking on Trigger Prebuild

Between the click and the moment a new Prebuild appears, there should be a visible clue that something is indeed actually happening behind the scenes (otherwise, users might think that the click didn't work).

Release Notes

NONE

/uncc

roboquat · 2021-09-17T07:13:40Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

Associated issue: #5374

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

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

roboquat · 2021-09-17T07:13:40Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

Associated issue: #5374

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

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

jankeromnes · 2021-09-17T14:11:19Z

Thanks for the new spinner @gtsiolis! Happy that it works equally well on light and dark, thus we can remove the extra theme-switching logic 😌

LIGHT	DARK

(Note: This is a drive-by fix, not really related to the Trigger Prebuild button 😇)

…build

jankeromnes · 2021-09-21T10:20:14Z

Okay, calling this ready for review (even if this could still be improved).

Current state of this PR

When you click on Trigger Prebuild in the Prebuilds page, the button gets disabled and receives a spinner:

NOT TRIGGERED	TRIGGERED

Then, when any one of these things happens (first one wins), the button gets re-enabled and removes the spinner:
a. A new prebuild is successfully triggered
b. There was already a prebuild for the latest commit on the default branch (so no new prebuild was triggered)
c. An error happens while triggering the prebuild
d. The button remained spinning for 30 seconds but nothing happened (UI timeout)

Caveats:

For b. there no visible feedback that a prebuild already existed. We could either add a visible feedback (e.g. highlight the latest prebuild), or change the server behavior (forcefully trigger a new prebuild even if a successful one already exists), see Rerunning Prebuild does nothing #5771 and relevant Slack thread (internal)
For c. and d. there is no visible feedback that something went wrong (but the error is logged to the Console). What should the UI do in this case? Show an window.alert() message? Or a toast notification?

jankeromnes · 2021-09-21T10:21:14Z

components/dashboard/src/projects/Prebuilds.tsx

+                    getGitpodService().server.triggerPrebuild(project.id, branchName),
+                    new Promise((_, reject) => setTimeout(() => reject(new Error('Timed out while waiting for new prebuild')), 30000)),
+                ]) as StartPrebuildResult;
+                console.log('Successfully triggerred!', result);


Note to self: Maybe I should remove this debug log.

svenefftinge · 2021-09-21T10:41:13Z

The spinner also starts when I "rerun prebuild" on one of the branches. I think it would be better if the indicator is set on the actual entry in the list. I.e. signal something before "Running" is shown.
Also not sure if we discussed this already, but the context-free, global "Trigger Prebuild" button leaves me wondering on what branch the prebuild will run. I'd vote for removing that button (cc @gtsiolis)

jankeromnes · 2021-09-21T10:47:02Z

Good points, thanks @svenefftinge!

For Trigger Prebuild, we could also add a "fantom Prebuild" entry (e.g. an empty/skeleton list item with some pending animation)
If we make Rerun Prebuild actually replace an older Prebuild, we could optimistically replace the Prebuild state just in the UI (e.g. "cosmetically" change "Ready" or "Failed" to "Pending" before even hearing back from the server, then actually adjust the UI when the server responds -- the Configurator does something similar to immediately respond to Run Prebuild clicks)

gtsiolis · 2021-09-21T11:10:27Z

Looking at this now! 👀

gtsiolis

Thanks @jankeromnes! Left some comments below! 💭

gtsiolis · 2021-09-21T11:52:41Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -150,7 +165,7 @@ export default function () {
                <div className="py-3 pl-3">
                    <DropDown prefix="Prebuild Status: " contextMenuWidth="w-32" entries={statusFilterEntries()} />
                </div>
-                <button disabled={!project} onClick={() => triggerPrebuild(null)} className="ml-2">Trigger Prebuild</button>
+                <button disabled={!project || isPrebuildPending} onClick={() => triggerPrebuild(null)} className="ml-2 flex items-center space-x-2"><span>Trigger Prebuild</span>{isPrebuildPending && <img className="h-4 w-4 animate-spin filter brightness-150" src={Spinner} />}</button>


suggestion: What do you think of placing the spinner on the left of the label? Spinner placement is a minor issue and we could go with any approach but I remember discussing something similar with the GitLab team in the past in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/26035#note_297307469. Cross-posting below the relevant comment for visibility within our team:

... regarding spinner placement, although there is not an ~~strict~~ established pattern or design guideline about this, it's mostly related to a) usability (screen reading) and b) LTR (left-to-right) v. RTL (right-to-left) languages where user interfaces should be mirrored to ensure content is easy to understand.

Since we currently only support LTR languages, there's no need to also add a component variant with the spinner on the right side. Once and if there is support for RTL languages we should consider adding a component variant with the spinner placed on the right.

BEFORE AFTER

gtsiolis · 2021-09-21T11:58:12Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -150,7 +165,7 @@ export default function () {
                <div className="py-3 pl-3">
                    <DropDown prefix="Prebuild Status: " contextMenuWidth="w-32" entries={statusFilterEntries()} />
                </div>
-                <button disabled={!project} onClick={() => triggerPrebuild(null)} className="ml-2">Trigger Prebuild</button>
+                <button disabled={!project || isPrebuildPending} onClick={() => triggerPrebuild(null)} className="ml-2 flex items-center space-x-2"><span>Trigger Prebuild</span>{isPrebuildPending && <img className="h-4 w-4 animate-spin filter brightness-150" src={Spinner} />}</button>


issue: Changing the state of this button whenever someone triggers an arbitrary prebuild form the prebuilds list, as mentioned in #5731 (comment), can be confusing to the user. I'd suggest to keep the state of the button intact instead.

gtsiolis · 2021-09-21T12:00:45Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -244,7 +259,7 @@ export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInst
                <span>PENDING</span>
                </div>;
            details = <div className="flex space-x-1 items-center text-gray-400">
-                <img className="h-4 w-4 animate-spin" src={props.isDark ? SpinnerDark : Spinner} />
+                <img className="h-4 w-4 animate-spin" src={Spinner} />


thought: Friendly reminder to come back to this if we ever adjust the spinner color used for light and dark themes. 🌔

gtsiolis · 2021-09-21T13:13:56Z

components/dashboard/src/projects/Prebuilds.tsx

@@ -150,7 +165,7 @@ export default function () {
                <div className="py-3 pl-3">
                    <DropDown prefix="Prebuild Status: " contextMenuWidth="w-32" entries={statusFilterEntries()} />
                </div>
-                <button disabled={!project} onClick={() => triggerPrebuild(null)} className="ml-2">Trigger Prebuild</button>


suggestion: Regarding the removal of the global Trigger Prebuild button mentioned in #5731 (comment) (Cc @svenefftinge), the linked issue (#5374) in the PR description contains some more context behind this:

From #5374 (comment):

In terms of UX, this button could trigger a modal to confirm prebuild on the default branch. Possible improvements could include selecting a different branch, passing an env var for a prebuild, etc.

From #5374 (comment):

We could skip this and remove the button for this iteration if you think it’s redundant. The idea behind this is that for projects that the latest prebuild failed and you’d like to retry or all the prebuilds have been removed after two weeks, someone could trigger a prebuild from the dashboard.

However, if we want to ship this sooner and we have the bandwidth to add a fix here, showing a spinner + disabling the button on click as you mentioned in #5374 (comment) sounds optimal. 🎯

I refrained from suggesting this earlier as there are also a few more places where a loading button component variant could be used for consistency, like the git integration modal, but we can progressively add it there later. ➿

🍊 🍊 🍊 🍊

Adding below some early design drafts how the confirmation modal could look like when triggering a prebuild from the global trigger prebuild button. Let me know if this does not look interesting or useful here. Using a modal here could allow us in the future to provide a way to insert one-time variables for a prebuild which, etc.

Run Prebuild Run Prebuild with Branch Selection

🍋 🍋 🍋 🍋

Regarding the replacement of an existing prebuild upon triggering a new prebuild mentioned in #5731 (comment) (Cc @jankeromnes), I'd think that ideally we'd never replace a prebuild but cancel the previous one and creating a new one. Maybe this is currently not technically possible or something we're interested in? 💭

jankeromnes · 2021-09-21T14:37:31Z

Thanks again for the reviews! 🙏

After several discussions, let's close this for now, and instead focus on #5791 😇

(I'll save the small Spinner-fix commit and add it to another PR)

jankeromnes added the approved label Sep 17, 2021

roboquat added do-not-merge/work-in-progress release-note-none labels Sep 17, 2021

roboquat added the size/S label Sep 17, 2021

gtsiolis added component: dashboard feature: prebuilds feature: teams and projects [DEPRECATED] Please, use feature: organizations or feature: projects labels instead. team: webapp Issue belongs to the WebApp team labels Sep 17, 2021

jankeromnes force-pushed the jx/trigger-prebuild-feedback branch from 838f091 to 838e415 Compare September 17, 2021 13:55

[dashboard] Fix spinner

3456d2c

jankeromnes force-pushed the jx/trigger-prebuild-feedback branch from 838e415 to 3456d2c Compare September 21, 2021 06:40

[dashboard] Disable & add spinner to button when triggering a new pre…

ceba53e

…build

roboquat added size/M and removed size/S labels Sep 21, 2021

jankeromnes marked this pull request as ready for review September 21, 2021 10:20

roboquat removed the do-not-merge/work-in-progress label Sep 21, 2021

jankeromnes commented Sep 21, 2021

View reviewed changes

jankeromnes requested review from gtsiolis, AlexTugarev and svenefftinge September 21, 2021 10:21

gtsiolis reviewed Sep 21, 2021

View reviewed changes

jankeromnes closed this Sep 21, 2021

jankeromnes mentioned this pull request Sep 21, 2021

[dashboard] Unify animated spinner for both light and dark themes #5792

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dashboard] Fix feedback when triggering prebuilds #5731

[dashboard] Fix feedback when triggering prebuilds #5731

jankeromnes commented Sep 17, 2021 •

edited

Loading

roboquat commented Sep 17, 2021

roboquat commented Sep 17, 2021

jankeromnes commented Sep 17, 2021 •

edited

Loading

jankeromnes commented Sep 21, 2021 •

edited

Loading

jankeromnes Sep 21, 2021

svenefftinge commented Sep 21, 2021

jankeromnes commented Sep 21, 2021 •

edited

Loading

gtsiolis commented Sep 21, 2021 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment

gtsiolis Sep 21, 2021

gtsiolis Sep 21, 2021

gtsiolis Sep 21, 2021

gtsiolis Sep 21, 2021

jankeromnes commented Sep 21, 2021 •

edited by werft-gitpod-dev-com bot

Loading

[dashboard] Fix feedback when triggering prebuilds #5731

[dashboard] Fix feedback when triggering prebuilds #5731

Conversation

jankeromnes commented Sep 17, 2021 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

roboquat commented Sep 17, 2021

roboquat commented Sep 17, 2021

jankeromnes commented Sep 17, 2021 • edited Loading

jankeromnes commented Sep 21, 2021 • edited Loading

Current state of this PR

jankeromnes Sep 21, 2021

Choose a reason for hiding this comment

svenefftinge commented Sep 21, 2021

jankeromnes commented Sep 21, 2021 • edited Loading

gtsiolis commented Sep 21, 2021 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

gtsiolis Sep 21, 2021

Choose a reason for hiding this comment

gtsiolis Sep 21, 2021

Choose a reason for hiding this comment

gtsiolis Sep 21, 2021

Choose a reason for hiding this comment

gtsiolis Sep 21, 2021

Choose a reason for hiding this comment

jankeromnes commented Sep 21, 2021 • edited by werft-gitpod-dev-com bot Loading

jankeromnes commented Sep 17, 2021 •

edited

Loading

jankeromnes commented Sep 17, 2021 •

edited

Loading

jankeromnes commented Sep 21, 2021 •

edited

Loading

jankeromnes commented Sep 21, 2021 •

edited

Loading

gtsiolis commented Sep 21, 2021 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes commented Sep 21, 2021 •

edited by werft-gitpod-dev-com bot

Loading