-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add basic Toast component #487
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
🦋 Changeset detectedLatest commit: e26bfa3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
I currently only have I would like to setup playwright for integration/browser tests and integrated into the CI pipeline, similar to shadcn-svelte and melt-ui's setup, but haven't had the time to tackle it yet. If that is an initiative you would like to get started, I would kindly review a PR 😁
Sadly most of the development occurs on Mac and thus a unix environment (and usually zsh or bash terminal). With that said, one of our contributors was using Windows and leveraging the WSL (he actually was the one to add the timezone environment variable, although he also switched to Mac later on). I don't hear of much Windows open source development happening without using WSL nowadays, but I'm also pretty detached from that ecosystem (switched to Mac 10+ years ago). If you're not using WSL, I would recommend setting it up and see if you have a better experience. |
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.
@willm0602 Thanks for the PR!
I left some notes, some of which might not be worth addressing in this PR, but thought it was helpful to put my thoughts down to see what future improvements, etc.
Other ideas we chatted about in Discord (putting here to more easily track) were:
- When you
pointerover
a notification, we should stop the timeout and onpointerout
we start/resume in case you are interacting with it (or just needed more time to read the message - Consider integrating Progress / ProgressCircle to show the countdown (probably best when we add the
variant
support)
.changeset/shaggy-roses-attack.md
Outdated
@@ -0,0 +1,5 @@ | |||
--- | |||
'svelte-ux': minor |
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.
If you do not select any packages when prompted for major
and minor
, it will produce a patch
changeset (I think the flow is a little odd to be honest, but that's driven by the changesets project).
The flow is different when using changesets
in a multi-package monorepo (like Svelte UX)...
vs a single-package repo (like LayerChart at the moment)...
The single package is much more straight forward.
At some point LayerChart might become multi-package.
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.
With that said you can manually edit this .md
file and just change minor
to patch
(all the CLI does is help you write this file, including generating a filename)
export type Toast = { | ||
text: string; | ||
timeAdded: Date; | ||
}; |
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.
Not to feature creep this too much, but also consider a little bit more future state, I'm thinking the the type could be:
export type Toast = {
title: string;
description?: string;
actions?: Record<string, Function>; // this will need more thought to support primary styling, etc
icon?: IconInput;
variant?: string;
timeAdded: Date;
};
variant
would need to wait til #86 is tackled, but it's not a big lift (mostly just taking the styles from the doc examples. See ToggleGroup for a semi-related example (there are other components with variant
as well).
*/ | ||
const toastStore = writable<Toast[]>([]); | ||
|
||
export function addToast(text: string, durationInMS = DEFAULT_TOAST_TIME_IN_MS) { |
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.
We would want to change text: string
into an object to support the various additional inputs (description, icon, actions, etc)
import Notification from './Notification.svelte'; | ||
</script> | ||
|
||
<div class="fixed bottom-0 left-1/2 translate-x-[-50%] z-50 gap-y-4"> |
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.
It would also be good if Toast
supported class overrides, either directly (<Toast classes={...}>
) or via settings.
To support this, you would need to add a classes
prop and also getting the classes defined in settings(...)
using...
<script>
...
export let classes: {
root?: string;
actions?: string;
// ...any other internal element/component you might want to pass a class to
} = {};
const settingsClasses = getComponentClasses('Toast');
</script>
and change...
<div class="fixed bottom-0 left-1/2 translate-x-[-50%] z-50 gap-y-4">
to
<div
class={cls(
'Toast',
'fixed bottom-0 left-1/2 translate-x-[-50%] z-50 gap-y-4',
settingsClasses.root,
classes.root,
$$props.class
)}
>
Few other ideas I forgot to mention. It might make sense to put an instance of the |
@techniq started working on the changes and got an issue where I can't get the toasters to appear over the elements of the app layout that used fixed position (so the side and topnav) even after adjusting the z-indexes Was wondering if you had any ideas for fixing this? |
Modal elements like Drawer and Dialog "portal" their content to the bottom of |
@willm0602 I started work on improving/simplifying the I also have a PR to use This will impact your PR some, but not sure where |
Adds a basic version of a toast component that wraps Notifications and includes them on the bottom
Wanted to also leave a couple of notes before this gets merged
'TZ' is not recognized as an internal or external command, operable program or batch file
. I modified the test script in package.json to beSET TZ=UTC+4 vitest
and that let the test suite run, but failed on six-issues, not sure if it's an issue with developing on windows OS