-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove re-run from /prebuilds, redirect to logs when rerunning #8835
Conversation
c0ab888
to
3b58e1e
Compare
This is good - thanks @laushinka . The only change I would suggest (and I know this is slightly different from what was discussed in #8765) would be to remove the context dots menu from the prebuilds list entirely, and link the whole list item to the prebuild detail. If that is a running prebuild, then the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small change in navigation behavior requested in the comment
Redirecting from the branches list to the prebuild detail, when a prebuild is started, is a nice improvement. |
3b58e1e
to
bf23c34
Compare
@gtsiolis I implemented a spinner but I think I could use your review here on the positioning 🙏🏽 |
It shows build failed but that's actually an old one 🤔 The preview env is green[1] |
Looking at this now! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping, @laushinka! 🏓
Left two comments below.
<div> | ||
{isLoading && ( | ||
<div className="flex justify-center"> | ||
<img alt="" className="h-4 w-4 animate-spin" src={Spinner} /> | ||
</div> | ||
)} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This placed spinner at the bottom of the branches list can definitely could*** cause some confusion, especially if the branches list is long, etc.
*** @laushinka Sorry if the initial wording here felt a bit strong. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right 🙏🏽 I'll find a different position for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: If we don't have a page we can render instantly I'd avoid the redirect in the first place.
<div> | ||
{isLoading && ( | ||
<div className="flex justify-center"> | ||
<img alt="" className="h-4 w-4 animate-spin" src={Spinner} /> | ||
</div> | ||
)} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Redirecting to the prebuild details page as @jldec described in #8835 (comment) sounds great, could we go with that approach? The prebuild details page already contains a loading indicator (spinner) at the bottom right? If for some reason the prebuild does not exist or the page is not available before some time we could introduce a new prebuild phase so that on trigger there's always a phase we can load and link to.
Let me now what you think, @laushinka! 💭
Loading indicator (spinner) on the prebuild details page |
---|
![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be ideal, unfortunately the spinner is necessary here because it takes a while until we get a response back to know which prebuild logs page to go to..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could not attempt to redirect until we have a page we can render in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could not attempt to redirect until we have a page we can render in time.
Hmm, how would the UX be like here? Also looping in @jldec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Showing a spinner near the action that the user clicked, and then redirecting after a pause, is still preferable over not redirecting at all.
6bfd371
to
fdbb269
Compare
/werft run with-vm=true 👍 started the job as gitpod-build-laushinka-dashboard-remove-action-8765.14 |
fdbb269
to
dc4d236
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Related Issue(s)
Fixes #8765
How to test
https://laushinka-5100f6fadc.staging.gitpod-dev.com/projects
Release Notes
Documentation