-
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
[dashboard] Update Project Configurator #5236
Conversation
/werft run from-scratch 👎 unknown command |
/werft run from-scratch 👎 unknown command |
/werft run with-clean-slate-deployment 👎 unknown command |
/werft run 👍 started the job as gitpod-build-jx-auto-gitpodify.16 |
b206df6
to
377eb8a
Compare
7ead16e
to
b045ea3
Compare
e2d4001
to
0529944
Compare
/werft run 👍 started the job as gitpod-build-jx-auto-gitpodify.28 |
/werft run 👍 started the job as gitpod-build-jx-auto-gitpodify.29 |
Looking a 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.
C'est magnifique @jankeromnes! 🔮
Thanks for pushing this forward and adding support for dark theme layouts! 🙇
Left a first round of comments. If you think any of these comments significantly increase the scope of this PR let's open follow up issues if needed.
{prebuildWasTriggered && <PrebuildStatus prebuildPhase={prebuildPhase} isDark={isDark} />} | ||
<div className="flex-grow" /> | ||
<button className="secondary">New Workspace</button> | ||
<button disabled={isEditorDisabled} onClick={buildProject}>Run Prebuild</button> |
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 is disabled when there's a git based configuration. Expected?
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 (it doesn't work as expected yet -- will open a follow-up issue to fix & enable it)
<div className="h-20 px-6 bg-gray-50 dark:bg-gray-800 border-t border-gray-200 dark:border-gray-600 flex space-x-2"> | ||
{prebuildWasTriggered && <PrebuildStatus prebuildPhase={prebuildPhase} isDark={isDark} />} | ||
<div className="flex-grow" /> | ||
<button className="secondary">New Workspace</button> |
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 does not perform any action but I guess this is expected here?! 🙈
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.
Also yes 😬
: <div className="flex-grow flex flex-col items-center justify-center"> | ||
<img className="w-14" role="presentation" src={isDark ? PrebuildLogsEmptyDark : PrebuildLogsEmpty} /> | ||
<h3 className="text-center text-lg text-gray-500 dark:text-gray-50 mt-4">No Recent Prebuild</h3> | ||
<p className="text-center text-base text-gray-500 dark:text-gray-400 mt-2 w-64">Edit the project configuration on the left to get started. <a className="learn-more" href="https://www.gitpod.io/docs/config-gitpod-file/">Learn more</a>.</p> |
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: The copy here is not accurate when project configuration is git based, right? What do you think of a) the following copy, b) using a slightly larger container width, and c) linking to the prebuilds docs instead? Also, let's remove the trailing dot from the help link for consistency with all other help links.
<p className="text-center text-base text-gray-500 dark:text-gray-400 mt-2 w-64">Edit the project configuration on the left to get started. <a className="learn-more" href="https://www.gitpod.io/docs/config-gitpod-file/">Learn more</a>.</p> | |
<p className="text-center text-base text-gray-500 dark:text-gray-400 mt-2 w-72">Prebuilds run on every commit and install dependencies, run build scripts, and more. <a className="learn-more" href="https://www.gitpod.io/docs/prebuilds/">Learn more</a></p> |
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'd feel a bit strange with the proposed new copy:
Prebuilds run on every commit
As a user discovering the Project Configurator, I'm now faced with a conflict: Should I click the "Run Prebuild" button, or push new commits instead as suggested here?
I don't think commits are relevant here, and even add confusion (either you're in a workspace, pushing commits, or you're in the Project Configurator and you test a config with a few clicks).
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 are probably right here! The intent here was to refrain from a) having duplicate content as the auto-detection message on the left[1][2] and b) showing an inaccurate message as editing the configuration is not possible for projects with a git-based configuration already in place. Instead this could help educate users about prebuilds.
What do you think of:
- Keeping the following on the empty state on the right:
Run prebuild or open a new workspace to edit project configuration.
- Changing the message on the left when the project has been detected successfully to something like the following:
The starter template below has been added based on the project type detected.
🍊 🍊 🍊 🍊
Also, enabling the Run Prebuild button would be great here, right?
</div> | ||
{isDetecting && <div className="absolute h-full w-full flex items-center justify-center space-x-2"> | ||
<img className="h-5 w-5 animate-spin" src={isDark ? SpinnerDark : Spinner} /> | ||
<span className="font-semibold text-gray-400">Detecting project type ...</span> |
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: When adding an empty repository, this is stuck into detecting loop. Could we use an error message and prompt the user to open a new workspace instead?
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.
When adding an empty repository, this is stuck into detecting loop.
Oh, that's unexpected & sounds like a bug. For me, an empty repository gets "detected" as "no setup found", and immediately gets the default config.
Did you notice any errors in the browser console?
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.
Did you notice any errors in the browser console?
Do not recall but will try again once the new preview env is up and running.
{prebuildWasTriggered && <PrebuildStatus prebuildPhase={prebuildPhase} isDark={isDark} />} | ||
<div className="flex-grow" /> | ||
<button className="secondary">New Workspace</button> | ||
<button disabled={isEditorDisabled} onClick={buildProject}>Run Prebuild</button> |
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: We could disable the run prebuild button when a prebuild is in progress. Alternatively, to avoid disabling buttons, which is a bad practice in terms of accessibility, we could trigger a modal to confirm a new prebuild and cancel the previous one in the background.
</div> | ||
{isDetecting && <div className="absolute h-full w-full flex items-center justify-center space-x-2"> | ||
<img className="h-5 w-5 animate-spin" src={isDark ? SpinnerDark : Spinner} /> | ||
<span className="font-semibold text-gray-400">Detecting project type ...</span> |
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: I could not get a detectable project up and running. 😭
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.
That's unexpected. @AlexTugarev seems to have gotten one in #5236 (comment) -- which project did you try?
Hint: Any project with a package.json
and without a .gitpod.yml
should get successfully detected (even if private).
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.
... which project did you try?
I remember trying a private repository with a starter template with Next.js.
e56682a
to
af8dfe2
Compare
af8dfe2
to
9a37f20
Compare
/werft run 👍 started the job as gitpod-build-jx-auto-gitpodify.42 |
/werft run 👍 started the job as gitpod-build-jx-auto-gitpodify.43 |
Okay, this is about as far as I'll get in this PR. @AlexTugarev could you please give this another look, and approve if it can be merged? I can adjust minor things and migrate all remaining open discussions to follow-up issues. Heads up: This refactors *Problems that make testing the logs almost impossible:
|
LGTM label has been added. Git tree hash: db3b657b23d0eb673221714f80590cfbd6eaddc5
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svenefftinge Associated issue: #4948 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:
Approvers can indicate their approval by writing |
For the record, the WorkspaceLogs refactor caused #5389 (thanks for catching this @AlexTugarev!) |
Addresses part of #4948 (just the final Configuration part)
Specs
#4948 (comment)
Details
checkX
setups in config-inferrer.ts)How to test
.gitpod.yml
) -- repo-based always has prioritypackage.json
)For each of these, try the Project Configurator.
init: yarn install
andcommand: yarn start
, clickRun Prebuild
Screenshots
/uncc