-
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
Adding Toasts #17030
Adding Toasts #17030
Conversation
started the job as gitpod-build-bmh-toasty.2 because the annotations in the pull request description changed |
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.
Excited to see the toast notifications coming alive, @selfcontained! 🌮 🌮
Left some UX comments below, but feel free to open follow-up issues or combine topics in a follow-up issue or PR. 🏓
Approving to unblock merging, but holding in case you'd like to address any of the comments in this PR.
/hold
onRemove: (id: string) => void; | ||
}; | ||
|
||
export const Toast: FC<Props> = ({ id, message, duration = 5000, autoHide = true, onRemove }) => { |
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.
praise: Excellent choice, 5 seconds. ✨
}, | ||
}, | ||
animation: { | ||
"toast-in-right": "toast-in-right 0.5s", |
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: Can we make sure the animation is ease-in
?
onRemove: (id: string) => void; | ||
}; | ||
|
||
export const Toast: FC<Props> = ({ id, message, duration = 5000, autoHide = true, onRemove }) => { |
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: Can we keep the toast shown if the user hovers the mouse over 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.
Nice idea, definitely!
clone and install your dotfiles for every new workspace. | ||
</p> | ||
</div> | ||
<InputField |
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.
praise: Thanks for reusing the form components here! 🙇
</div> | ||
<InputField | ||
label="Repository URL" | ||
hint="Add a repository URL that includes dotfiles. Gitpod will clone and install your dotfiles for every new workspace." |
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.
Good eye 🦅 👁️ - it was a side effect of re-using the form components here, and finding a way to add the button into the row the input was on, but keep the hint
rendering as it did by default. I'll play around with this a little more to see if there's a better way to have that behavior, but still re-use the form input components.
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, I see. Yeah, let's make this change if it's trivial—otherwise we could keep it like this, as I wouldn't be surprized if the UX or interaction to update the dotfiles or timeouts moves away from these inline forms. Your call. 🏓
See relevant comments:
</div> | ||
<InputField | ||
label="Repository URL" | ||
hint="Add a repository URL that includes dotfiles. Gitpod will clone and install your dotfiles for every new workspace." |
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.
}, | ||
}, | ||
animation: { | ||
"toast-in-right": "toast-in-right 0.5s", |
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: Can we make the toast ease-in from the bottom?
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.
Yah we can. fwiw I tried coming in from the bottom and centered, and it looked a little janky w/ stacked toasts and how they would shift up to make room instantly while the new one slides up. We could make it pretty slick, but will be a bit more effort to animate it nicely I think. In from right looks a little better with this lower level of effort imo, so I ended up doing that. It's not too tough to swap it to in from bottom to show you what I mean sometime too if you wanted to see each.
className={classNames( | ||
"relative flex justify-between items-center", | ||
"w-full md:w-96 max-w-full", | ||
"p-6 border-1 md:rounded", |
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: Could we make the design of the toast closer to the original design spec in #3530? For example, larger corner radius, smaller padding, better color text for accessibility, bolder x icon. Later on we can explore how toasts with two lines of text or actions could look like.
BEFORE | AFTER |
---|---|
My plan was to make that an error styled toast, but in a follow up where I can work with @gtsiolis around what those styled toasts might look like. It would be easy to make that a toast instead in this PR too though. |
Regarding the browser alert[1][2], I'd think it should be perfectly fine to not fix this in the scope of this PR but I'll leave this to the engineering team to decide. @selfcontained Your call, but let me know if you need any UX input on this now or later. 💭 Thanks again for making this happen! 🍞 |
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 explanations. 🙏
Code LGTM, tested and works. 🧘
Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>
Co-authored-by: George Tsiolis <tsiolis.g@gmail.com>
/unhold |
Description
WIP
Adding Toasts and updating User Preferences to use them for dotfiles and timeout fields. Also tweaked those forms on user prefs page to have a bit more feedback for the UX.
Some considerations:
Related Issue(s)
Fixes WEB-38
Fixes #3530
How to test
Release Notes
Documentation
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh