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

docs(notification): add notification code docs #1217

Merged
merged 16 commits into from
Jun 17, 2020
Merged

docs(notification): add notification code docs #1217

merged 16 commits into from
Jun 17, 2020

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented May 22, 2020

Updates Notification code docs according to (my interpretation) of our template.

@dakahn dakahn requested a review from a team May 22, 2020 02:45
@dakahn dakahn requested a review from a team as a code owner May 22, 2020 02:45
@dakahn dakahn requested review from connor-leech and removed request for a team May 22, 2020 02:45
@netlify
Copy link

netlify bot commented May 22, 2020

Deploy preview for carbon-website ready!

Built with commit 2b1a949

https://deploy-preview-1217--carbon-website.netlify.app

@vercel
Copy link

vercel bot commented May 22, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-design-system/carbon-website/cga9xtxjz
✅ Preview: https://carbon-website-git-fork-dakahn-notification-usage-updates.carbon-design-system.vercel.app

@ghost ghost requested review from asudoh and tw15egan and removed request for a team May 22, 2020 02:45
@dakahn
Copy link
Contributor Author

dakahn commented May 22, 2020

Removing WIP label since it seems like it all built fine and dandy

@vercel vercel bot temporarily deployed to Preview May 22, 2020 09:51 Inactive
@dakahn dakahn changed the title WIP docs(notification): add notification code docs docs(notification): add notification code docs May 22, 2020
@dakahn dakahn requested a review from joshblack May 27, 2020 16:08
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looking great so far!

src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
src/pages/components/notification/code.mdx Show resolved Hide resolved

## Toast notification

You can use the `ToastNotification` component to display a non-modal, time-based short message that appears at the bottom or the top of the screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be a non-modal dialog?

Also, just realized that we don't provide any kind of layout for where this component should be rendered 🤔

Copy link
Contributor Author

@dakahn dakahn May 27, 2020

Choose a reason for hiding this comment

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

Like do you mean we just say "render at the top" but don't describe how to do that?

src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
src/pages/components/notification/code.mdx Show resolved Hide resolved
src/pages/components/notification/code.mdx Show resolved Hide resolved
src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview May 27, 2020 22:55 Inactive
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Seems great! Just that one comment

src/pages/components/notification/code.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@janchild janchild left a comment

Choose a reason for hiding this comment

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

@dakahn there are just two things here from a content perspective:

  1. Update the page description as follows:
    How to build a notification using React. For code usage with other frameworks, please follow the links in the live demo on the usage tab.

  2. The anchor links are out of sync.

Thanks!

@vercel vercel bot temporarily deployed to Preview June 5, 2020 22:11 Inactive
@dakahn
Copy link
Contributor Author

dakahn commented Jun 5, 2020

Updates added! Should be good to go 🏄

@janchild
Copy link
Contributor

janchild commented Jun 8, 2020

@dakahn , there is one H2 that's not currently in the anchor links: Inline notifications with action button. And it looks like it should probably be positioned after the H2 Inline notifications section, with the Component API and the Feedback H2s following behind. Is that right? I might be missing something here...

So I'm thinking it should be:

  • Toast notification
  • Inline notification
  • Inline notifications with action button
  • Component API
  • Feedback

Thanks

@joshblack
Copy link
Contributor

Should be good to go for another round of reviews 👀

@janchild
Copy link
Contributor

Sorry, I should have been clearer. @dakahn @joshblack

The ordering of the anchor links still doesn't match the H2s on the page.
Plus it looks like the topic of Inline notifications with action button should probably be positioned directly after the H2 Inline notifications section, with the Component API and the Feedback H2s following behind. Is that right? Could you please check this?

If what I'm suggesting is correct, then the anchor links should be all the H2s in this order:

  • Overview
  • Toast notification
  • Inline notification
  • Inline notifications with action button
  • Component API
  • Feedback

Thanks!

@vercel vercel bot temporarily deployed to Preview June 10, 2020 22:57 Inactive
@dakahn
Copy link
Contributor Author

dakahn commented Jun 10, 2020

@janchild re organized the H2s and added the Overview anchor back in 👍

Copy link
Contributor

@janchild janchild left a comment

Choose a reason for hiding this comment

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

🎉 Thanks, @dakahn !!

@joshblack joshblack merged commit c2ba72e into carbon-design-system:master Jun 17, 2020
natashadecoste pushed a commit to natashadecoste/carbon-website that referenced this pull request May 19, 2021
…1217)

* docs(notification): add notification code docs

* docs(notification): remove skeleton anchor link

* docs(Notification): several suggested PR changes

* fix(docs): update notification code docs

* docs(notification): sync up the anchor links

* docs(notification): fix missing anchor

* docs(notification): add overview anchor and reorder h2

Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Jan Child <[email protected]>
natashadecoste pushed a commit to natashadecoste/carbon-website that referenced this pull request May 19, 2021
…1217)

* docs(notification): add notification code docs

* docs(notification): remove skeleton anchor link

* docs(Notification): several suggested PR changes

* fix(docs): update notification code docs

* docs(notification): sync up the anchor links

* docs(notification): fix missing anchor

* docs(notification): add overview anchor and reorder h2

Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Jan Child <[email protected]>
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.

3 participants