-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[prebuilds] prebuild detail view in create workspace #10696
Conversation
8386a8f
to
4ae8ffd
Compare
a10b043
to
b6d2007
Compare
/werft run 👍 started the job as gitpod-build-laushinka-prebuild-view-9132.12 |
started the job as gitpod-build-laushinka-prebuild-view-9132.11 because the annotations in the pull request description changed |
eb1689a
to
26b43b6
Compare
26b43b6
to
68eeca1
Compare
9921c71
to
2fac644
Compare
dd3199e
to
d2c08f3
Compare
This is basically done, and I'd love to approve. But as we found out above, we're missing proper resourceguards for prebuilds. So I'm going to hold this PR, and try to get it in before we land this. /hold |
Now waiting for #10940 to get merged... |
/werft run 👍 started the job as gitpod-build-laushinka-prebuild-view-9132.48 |
Looking at this now! 👀 |
d2c08f3
to
8f2b6c8
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.
I never clicked submit but here's what I had in draft since yesterday. 🙈
Since we do not have yet the capability to expand the logs into a full page component, going with the current logs width sounds ok. Any thoughts on #9132 (comment)?
Two issues that pop up:
- The scrollbar position mentioned in [prebuilds] prebuild detail view in create workspace #10696 (comment). Moving forward with the current scrollbar styles sounds ok but the positioning the scrollbar in the middle of the element feels like something is broken.
- The width of the element wrapping the bottom parts of the prebuilds logs differ per case. Could we adjust the width of the bottom elements to match the prebuilds logs element above? See screenshots below.
Prebuild (Running) | Prebuild (Preparing) |
---|---|
/werft run 👍 started the job as gitpod-build-laushinka-prebuild-view-9132.50 |
started the job as gitpod-build-laushinka-prebuild-view-9132.51 because the annotations in the pull request description changed |
72e292b
to
5f80c29
Compare
@gtsiolis I think I fixed it (by re-using the same horizontal spacing we had in Thx @jankeromnes for the help! 🙏
|
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.
Tested and works! 👍
/hold in cases someone else wants to review
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.
Perfection achieved. ✨
Thanks, @geropl, @laushinka, @jldec, and @jankeromnes! 🏀
While I still think using a narrower width as described in #9132 (comment) could be interesting to explore,
Looking forward for #10041 or whatever comes next.
import CodeText from "../components/CodeText"; | ||
import FeedbackComponent from "../feedback-form/FeedbackComponent"; | ||
import { isGitpodIo } from "../utils"; | ||
|
||
const WorkspaceLogs = React.lazy(() => import("../components/WorkspaceLogs")); |
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.
I wonder if PrebuildLogs
should also be lazy-loaded here?
E.g. no need to load all of xTermjs if you're creating a workspace that doesn't show any logs.
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.
Ah, good idea! But my workspace has timed out, so: let's do as a follow-up 😉
/unhold |
Description
This PR gives a perceived consistency between the create workspace page and the prebuild detail view.
Related Issue(s)
Fixes #9132
How to test
Release Notes
Documentation