Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Contrib > Toast Component #1676

Merged
merged 69 commits into from
May 21, 2018
Merged

Conversation

Blackbaud-SteveBrush
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush commented May 10, 2018

Original contribution: #1614

Addresses: #163

The following changes were made:

  • Numerous style adjustments that were decided upon a few days ago. (Happy to provide more details around this!)
  • Flattened toast folder structure to be more consistent with other modules.
  • Moved component injection logic from the SkyToastComponent to the SkyToasterComponent. This was done to make the Toast component behave more like a "normal" component, with inputs, outputs, and transcluded content. The toaster handles both custom and default component injection for the toast's inner content (meaning, the toast component itself now only needs to worry about its core features).
  • Renamed the toast service's public method openTemplatedMessage to openComponent, since the word "template" has special meaning in Angular, and to further separate the usage of message from the openMessage method (both methods still operate in the same manner as they did before).
  • Created a new class SkyToast which holds the configuration aspects of each toast, and slightly modified SkyToastInstance to be the publicly consumable "interface" returned from the service methods. (Most of what used to be in SkyToastInstance was moved to SkyToast.) This will allow us to wrap internal-only utilities in SkyToast (for the Toaster component), and only provide what we want the consumer to see in SkyToastInstance.

blackbaud-conorwright and others added 30 commits March 7, 2018 17:17
@codecov-io
Copy link

codecov-io commented May 10, 2018

Codecov Report

Merging #1676 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1676      +/-   ##
==========================================
+ Coverage   99.98%   99.98%   +<.01%     
==========================================
  Files         395      406      +11     
  Lines        8145     8354     +209     
  Branches     1198     1223      +25     
==========================================
+ Hits         8144     8353     +209     
  Misses          1        1
Impacted Files Coverage Δ
src/modules/animation/slide.ts 100% <ø> (ø) ⬆️
src/modules/toast/toast-instance.ts 100% <100%> (ø)
src/modules/toast/toast-body.component.ts 100% <100%> (ø)
src/modules/toast/toast.component.ts 100% <100%> (ø)
src/modules/toast/toast.ts 100% <100%> (ø)
src/modules/toast/toast.module.ts 100% <100%> (ø)
src/modules/toast/toast.service.ts 100% <100%> (ø)
src/modules/toast/toaster.component.ts 100% <100%> (ø)
src/modules/toast/types/toast-type.ts 100% <100%> (ø)
src/modules/toast/toast-body-context.ts 100% <100%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7db88e8...8357564. Read the comment docs.

Copy link

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked through all the code yet, but saw a couple things I wanted to bring up. I left a few inline comments and have included some generic ones below.

  • There's a hover style on the toasts that I don't see in SKY UX 1. If it is intentional, it seemed blurry.
  • SKY UX 1 version has an opening and closing animation, whereas I only saw a close animation on this version.
  • If you have more toasts open than the height of the viewport, there's no way to get to them. This behavior exists in SKY UX 1 version and is definitely an edge-case, but thought I should still mention it.
  • Is there any value in exposing the SkyToastType as an enum, so you can do something like SkyToastType.info?
  • It's not entirely clear what the relationship is between toast and toaster components.

-khtml-user-select: none; /* Konqueror HTML */
-moz-user-select: none; /* Firefox */
-ms-user-select: none; /* Internet Explorer/Edge */
user-select: none; /* Chrom and Opera */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome is misspelled.

padding-bottom: $sky-margin-double;
padding-right: $sky-margin-double;
max-width: 300px;
-webkit-touch-callout: none; /* iOS Safari */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we wouldn't want users to be able to select text inside a toast?

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-BobbyEarl In response to your comments (and for posterity):

  • The animation should use "emerge/recede", as described here (I'll implement these changes ASAP): https://developer.blackbaud.com/skyux/design/styles/motion
  • I've changed the string type to enum, as suggested.
  • I've removed the hover state, and the user-select.
  • Let's chat offline about the relationship between toast and toaster.

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-BobbyEarl I added an "emerge" animation, as described here: https://developer.blackbaud.com/skyux/design/styles/motion. Also, I added an overflow style to the toaster component to handle a large number of toasts.

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit 65824da into master May 21, 2018
@Blackbaud-SteveBrush Blackbaud-SteveBrush deleted the contrib-toastr-fromscratch branch May 21, 2018 18:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants