Skip to content
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

Toasts v2.1 #834

Merged
merged 15 commits into from
Jul 17, 2019
Merged

Toasts v2.1 #834

merged 15 commits into from
Jul 17, 2019

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Jul 3, 2019

This is on top of #831 and adds a few tweaks. The diff might be hard to follow, it's mostly:

  • Renames Toast-octicon to Toast-icon. Not really necessary, but makes it a bit more generic.
  • Uses a <button> to dismiss instead of a <a>. This will still need some JS to make it work.
  • Removes the Toast-block-default modifier class so that by default, the icon is blue.
  • Removes block from the modifier classes. Also uses -- double dash -> Toast--error. I think this is part of our naming convention. Like UnderlineNav--right.
  • Splits the animation into Toast--animateIn and Toast--animateOut. So they can be triggered by JS.

@vercel
Copy link

vercel bot commented Jul 3, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@simurai simurai requested a review from emilybrick July 3, 2019 15:08
Copy link
Contributor

@ampinsk ampinsk left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks for carrying this along @simurai and @emilybrick! ✨ I just noticed one thing when I was checking out storybook. What do we need to do to finish this up? I can help clean up and write the documentation. Anything else?

@@ -88,8 +88,8 @@ To create a default toast, use `.Toast`

```html title="Toast animating"
<div class="p-3">
<div class="Toast Toast-animated Toast-block-error">
<span class="Toast-octicon">
<div class="Toast Toast--error Toast--animateIn">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the animation be a separate class? Or should they all automatically animate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think animation should be optional. Also, I think the classname should be Toast--animate-in, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

@simurai and I chatted last week about the animation being a modifier, I think it makes more sense to leave it up to the user in that regard (how/when to add the class). Unsure about the classname, defer to what you think is best @shawnbot @simurai

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the animation be a separate class?

Maybe we can revisit this after we see 2-3 use cases. It also depends a bit how a Toast gets added to the page. On page load and initially hidden or injected only when needed?

@shawnbot I think the classname should be Toast--animate-in, no?

Our naming convention doesn't specify the format when using multiple words for a modifier. So my thinking was to use camelCase to not make it look like the modifier has a child element. SUIT CSS uses the same convention.

@ampinsk
Copy link
Contributor

ampinsk commented Jul 15, 2019

I updated the documentation to use the erb way of including octicons (aka <%= octicon "info" %>), but now none of the examples are visible. The code is more readable and accurate this way, but it's kind of a bummer not seeing the examples. Anyone have thoughts?

@emilybrick
Copy link
Contributor

The code is more readable and accurate this way, but it's kind of a bummer not seeing the examples. Anyone have thoughts?

+1 here. Even on other pages, it's been difficult to use these docs without examples.

@simurai
Copy link
Contributor Author

simurai commented Jul 16, 2019

The code is more readable and accurate this way, but it's kind of a bummer not seeing the examples. Anyone have thoughts?

Yeah, erb currently doesn't work for rendering the examples. 😞 There is an idea about using <g-octicon name="info" /> https://github.com/primer/css/issues/815.

As of today, I guess we have the following options:

  1. Use erb and not show a live example. -> But that would remove a lot of the value.
  2. Use an image to show how the component looks. -> Nope, would be a pain to maintain, let's not go there. 😅
  3. Use html and wrap <%= octicon "info" %> in a comment. -> Examples would have no icons and people have to "uncomment" the octicon when using on .com.
  4. Use html and inline the <svg> -> the code becomes bloated and people need to remember to use <%= octicon "info" %> instead.

Hmmm.. all the options are not that great. 🤔 How about option 5 that is somewhat a combination of 3+4:

<span class="Toast-icon">
  <!-- <%= octicon "info" %> -->
  <svg class="octicon octicon-info" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M6.3 5.69a.942.942 0 0 1-.28-.7c0-.28.09-.52.28-.7.19-.18.42-.28.7-.28.28 0 .52.09.7.28.18.19.28.42.28.7 0 .28-.09.52-.28.7a1 1 0 0 1-.7.3c-.28 0-.52-.11-.7-.3zM8 7.99c-.02-.25-.11-.48-.31-.69-.2-.19-.42-.3-.69-.31H6c-.27.02-.48.13-.69.31-.2.2-.3.44-.31.69h1v3c.02.27.11.5.31.69.2.2.42.31.69.31h1c.27 0 .48-.11.69-.31.2-.19.3-.42.31-.69H8V7.98v.01zM7 2.3c-3.14 0-5.7 2.54-5.7 5.68 0 3.14 2.56 5.7 5.7 5.7s5.7-2.55 5.7-5.7c0-3.15-2.56-5.69-5.7-5.69v.01zM7 .98c3.86 0 7 3.14 7 7s-3.14 7-7 7-7-3.12-7-7 3.14-7 7-7z"></path></svg>
</span>

Then the examples show an icon and it's hopefully obvious enough that the <%= octicon "info" %> should be used on .com instead of the <svg>?

@simurai
Copy link
Contributor Author

simurai commented Jul 16, 2019

What do we need to do to finish this up?

Oh.. just remembered two more questions that came up when chatting with @emilybrick + @zainkho.

  1. Currently a Toast doesn't have any styles for positioning. Utilities like Toast position-fixed bottom-0 can be used, but it depends on ...
  2. What should happen when there are multiple Toasts at the same time? Initially we could keep it simple and just let multiple Toasts stack on top of each other.

If so, some sort of wrapper element is needed, like Toast-wrapper. But that looks more like it's a child element of the Toast component. @zainkho had the funny idea of calling it Toaster. 🤣 An alternative would be to call the "wrapper" Toast. It can have one or multiple Toast-items.

<div class="Toast">
  <div class="Toast-item">...</div>
  <div class="Toast-item">...</div>
  <div class="Toast-item">...</div>
</div>

Co-Authored-By: Shawn Allen <[email protected]>
`style="fill:currentcolor"` can be used for inline `<svg>`s
@emilybrick
Copy link
Contributor

I don't think we need to be overly cautious about multiple toasts at this stage.
Stacking is fine for now. I guess better to build in the way that's most scalable in the future, e.g.:

<div class="Toast">
  <div class="Toast-item">...</div>
  <div class="Toast-item">...</div>
  <div class="Toast-item">...</div>
</div>

In the spirit of incremental correctness and keeping this PR lean (so that logbook can use it in the near future), I'd like to propose we leave positioning as utilities. Eventually I do believe this should be in the component (e.g. toasts should always be in the bottom left on desktop 🤷‍♂), but would like to get this merged in this week. Open to thoughts @ampinsk @shawnbot @simurai


Regarding examples, option 5 seems fine to me. I prefer having the examples be in the docs to start, even if we'll eventually need to clean up the longhand. This seems 🆗:

<span class="Toast-icon">
  <!-- <%= octicon "info" %> -->
  <svg class="octicon octicon-info" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M6.3 5.69a.942.942 0 0 1-.28-.7c0-.28.09-.52.28-.7.19-.18.42-.28.7-.28.28 0 .52.09.7.28.18.19.28.42.28.7 0 .28-.09.52-.28.7a1 1 0 0 1-.7.3c-.28 0-.52-.11-.7-.3zM8 7.99c-.02-.25-.11-.48-.31-.69-.2-.19-.42-.3-.69-.31H6c-.27.02-.48.13-.69.31-.2.2-.3.44-.31.69h1v3c.02.27.11.5.31.69.2.2.42.31.69.31h1c.27 0 .48-.11.69-.31.2-.19.3-.42.31-.69H8V7.98v.01zM7 2.3c-3.14 0-5.7 2.54-5.7 5.68 0 3.14 2.56 5.7 5.7 5.7s5.7-2.55 5.7-5.7c0-3.15-2.56-5.69-5.7-5.69v.01zM7 .98c3.86 0 7 3.14 7 7s-3.14 7-7 7-7-3.12-7-7 3.14-7 7-7z"></path></svg>
</span>

Curious your thoughts @ampinsk ☝️

@shawnbot
Copy link
Contributor

Lemme see if I can get <g-octicon> working in the examples off this branch; if so, we can merge into this PR and update the example accordingly. 🤞

@ampinsk
Copy link
Contributor

ampinsk commented Jul 16, 2019

Agree with all your proposals @emilybrick! Incrementally correct away 🚀

@broccolini
Copy link
Member

Lemme see if I can get working in the examples off this branch; if so, we can merge into this PR and update the example accordingly. 🤞

@shawnbot If you are able to get this working in the next day or so that would be helpful, otherwise I suggest we ship with the SVG so that the example looks good (you could use an image tag instead of the svg inline if that's better for the code example). I don't feel strongly though so whatever works to ship this week.

In the spirit of incremental correctness and keeping this PR lean (so that logbook can use it in the near future), I'd like to propose we leave positioning as utilities.

I agree with @emilybrick that we don't need to worry about the container at this stage, use utilities for now and abstract later. I think we'll definitely iterate on this component wants it's had more testing in the real world. @simurai I'd love for this v1 to be shipped this week since @ampinsk is ramping back onto product work soon!

Eventually I do believe this should be in the component (e.g. toasts should always be in the bottom left on desktop

I'd suggest including this information in the docs, this may also be part of design guidelines in future (along with other alert message usage guidelines), however it's fine if this is in a follow up PR.

@shawnbot
Copy link
Contributor

@shawnbot If you are able to get this working in the next day or so that would be helpful, otherwise I suggest we ship with the SVG so that the example looks good (you could use an image tag instead of the svg inline if that's better for the code example). I don't feel strongly though so whatever works to ship this week.

I'm sorry, this turns out to be trickier than I'd anticipated. Blueprints' CodeExample component doesn't let us embed scripts in the rendered code example iframes, and it doesn't "inherit" scripts from the parent document either. This might need to wait until we migrate to the new docs tools that @colebemis is working on. ☹️

@simurai
Copy link
Contributor Author

simurai commented Jul 16, 2019

In the spirit of incremental correctness and keeping this PR lean (so that logbook can use it in the near future), I'd like to propose we leave positioning as utilities.

👍 Yep, totally fine.

Ok, I'll make the following changes:

  1. Use <!-- <%= octicon "info" %> --> <svg>... for now.
  2. Add an extra wrapper <div> to fix the scrolling of the examples.
  3. Add an example about positioning a toast with utility classes.
  4. Merge this PR into toasts-v2.

simurai added 3 commits July 17, 2019 10:42
This is needed so that the Toast's margin's don't get collapsed and the example gets cropped.
@emilybrick
Copy link
Contributor

ty @simurai ! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants