-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
Use toast to show app notifications #975
Conversation
I like this approach much better. Thank you for taking time out of your schedule to put this together. |
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 prefer the original alert style since it matches the rest of the site but overall this looks good. Is there a way to work the logo into this design too?
I'm not sure if this is by design but clicking anywhere on the toast closes it, not just the X
button.
The changes also look to play well with GitHub Dark. The warning style needs the icon color changes, but I can fix that in GitHub Dark once this is merged.
Looks like I was wrong, the toast design does come from github. Now if I could only get the site to show me one. |
This maybe caused by the click listener which is supposed to close the toast when following a link from there. I'll look into it. |
@xt0rted I pushed a single fixup commit with a bunch improvements.
|
That logo addition is adorable. |
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.
Co-authored-by: Brian Surowiec <brian@13degrees.com>
packages/core/notification.js
Outdated
} | ||
} | ||
}); | ||
const releaseDescription = 'Test'; |
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.
Every time the new version notification shows it's prefixed with Test -
as you can see in my previous screen shots. Should this be changed/removed?
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.
The idea was rather than to make an API call to bake the release description into the release version itself. Obviously this requires updating this string, which can slip through easily. Let me see if I can find a way to fail the build in such a case.
This looks good now, only the close button or a link in the toast closes it. I think the new version message needs to be adjusted before this can be merged though. |
True, will update the message before releasing a new version |
First attempt to tweak the app notification to become less intrusive. This toast message appears in the bottom left corner and therefore doesn't cause the page jump when showing the notification.
Screenshots