-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[dashboard] Next steps nudge for local-preview #11434
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
Conversation
started the job as gitpod-build-tar-lp-out-nudge.2 because the annotations in the pull request description changed |
/werft with-preview 👎 unknown command: with-preview |
/werft run 👍 started the job as gitpod-build-tar-lp-out-nudge.4 |
1c02d1c
to
1741d68
Compare
1741d68
to
5450358
Compare
3416066
to
a4317e8
Compare
Screenshot of what a user would see @gtsiolis Looking forward to your feedback 👀 |
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 picking this up, @Pothulapati! 🔮
Left one issue below to make this UX ready, and one suggestion for updating the copy.
Approving to unblock merging but holding to let someone from the @gitpod-io/engineering-webapp
take a closer look at the code changes.
/hold
components/dashboard/src/Menu.tsx
Outdated
@@ -489,6 +493,18 @@ export default function Menu() { | |||
))} | |||
</nav> | |||
)} | |||
{isLP && ( |
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.
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.
@gtsiolis Can you take a look on this layout issue?
I've moved this into App.tsx
where the new AppNotifications are, and you can access the instance at https://tar-lp-out-nudge.preview.gitpod-dev.com/workspaces
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.
Looks great, @Pothulapati! Two minor issues:
- Grid: We're using a page container with a fixed layout for our grid layout. Using this approach the ideal maximum width of the page container should follow the fixed width of the page container.
- Spacing: We've been using an 8-point grid system with spacing scale to help us provide all the negative area needed between elements and components, see relevant discussion (internal). Every part of the user interface should be intentional about empty space between elements. The amount of space between items creates relationships and hierarchy. For example,
8px
is used to separate related elements while16px
is used to separate unrelated elements.
Cc @gitpod-io/engineering-self-hosted for visibility and future reference.
One interesting point we may run into the future could be to see how this alert stacks with more global alerts coming through the AppNotifications.txt
, which could be fine as long as we limit the amount of alerts we stack, but I'd assume there's little to zero changes of running into more alerts here as the local preview is not intended for production use.
To get back to the actual UX changes, using a parent div
container for the alert with two classes (app-container
and mt-2
) should make all the necessary changes here. 🍵
BEFORE | AFTER |
---|---|
![]() |
![]() |
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 assume there's little to zero changes of running into more alerts here as the local preview is not intended for production use.
Exactly, and hence my reason for not using AppNotifications
.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
62e305d
to
01c1175
Compare
2ec28e7
to
144634b
Compare
144634b
to
a11e6ee
Compare
Currently, After `local-preview` is exited There are no direct concrete steps for users to take. This PR fixes this by adding a new `Alert` box to the global dashboard if we notice that they are on a Gitpod `local-preview` DOMAIN, which is `preview.gitpod-self-hosted.com` (our own DOMAIN) from now. Signed-off-by: Tarun Pothulapati <[email protected]>
a11e6ee
to
984f9d2
Compare
Description
Currently, After
local-preview
is exited There are nodirect concrete steps for users to take.
This PR fixes this by adding a new
Alert
box to theglobal dashboard if we notice that they are on a Gitpod
local-preview
DOMAIN, which ispreview.gitpod-self-hosted.com
(our own DOMAIN) from now.
Signed-off-by: Tarun Pothulapati [email protected]
Signed-off-by: Tarun Pothulapati [email protected]
Related Issue(s)
Part of #11381
How to test
Run
and see the nudge being present.
Release Notes
Documentation
Werft options: