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

[BD-46] feat: add new toast #2959

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

khudym
Copy link
Contributor

@khudym khudym commented Dec 21, 2023

Description

Implemented new toast based on event emitter
Issue#1858

Deploy Preview

components/toast

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Dec 21, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @khudym!

When this pull request is ready, tag your edX technical lead.

Copy link

netlify bot commented Dec 21, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 21168db
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/6595448fbd76f100080b954c
😎 Deploy Preview https://deploy-preview-2959--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -2,7 +2,8 @@
title: 'Toast'
type: 'component'
components:
- Toast
- ToastContainer
- toast
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Is toast a React component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i`ll remove it, not a component

</Toast>

<Button variant="primary" onClick={() => setShow(true)}>Show Toast</Button>
<div className="mt-3">
Copy link
Contributor

Choose a reason for hiding this comment

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

[important] Let's shorten the example of using the component and use a ready-made ExamplePropsForm component, which we use to improve the user experience.

For example:

  1. We can display the number of milliseconds as a range
  2. Add position options as inputs with the radio type
  3. Use a checkbox to display the action

Example of using the ExamplePropsForm component:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

ToastContainer.propTypes = {
/** Specifies contents of the component. */
children: PropTypes.node.isRequired,
position: PropTypes.oneOf(['top-left', 'top-right', 'bottom-left', 'bottom-right']),
Copy link
Contributor

Choose a reason for hiding this comment

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

[important] let's put the PropTypes value into a constant

For example: const TOAST_POSITIONS = ['top-left', 'top-right', 'bottom-left', 'bottom-right'];

export const TOAST_CLOSE_LABEL_TEXT = 'Close';
export const TOAST_DELAY = 5000;
import Button from '../Button';
import { Close } from '../../icons';

function Toast({
Copy link
Contributor

Choose a reason for hiding this comment

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

[proposal] I propose renaming this component to BaseToast. This will make it more obvious what it is used for.

>
<div className="toast__header small">
<p className="toast__message">{message}</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Do we need this gap?

Copy link
Contributor Author

@khudym khudym Jan 2, 2024

Choose a reason for hiding this comment

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

i`ll remove it

src/index.js Outdated
@@ -133,7 +133,8 @@ export {
TabPane,
} from './Tabs';
export { default as TextArea } from './TextArea';
export { default as Toast, TOAST_CLOSE_LABEL_TEXT, TOAST_DELAY } from './Toast';
export { default as ToastContainer } from './Toast/ToastContainer';
export { toast } from './Toast/toast';
Copy link
Contributor

Choose a reason for hiding this comment

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

[important] I suggest changing the file structure in the Toast folder.

Let's consider the option of moving the EventEmitter.js and toast.js files to the utils folder (+ we will implement re-export for these files). Let's rename the index.jsx file to BaseToast, and ToastContainer.jsx in index.jsx.

For example:

  • Toast
    • tests
      • ...
    • utils
      • EventEmitter.js
      • index.js
      • toast.js
    • _variables.scss
    • BaseToast.jsx
    • index.scss
    • README.MD
    • index.jsx

The export from Paragon will look like this:
export { default as ToastContainer, toast } from './Toast';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, i`ll do it. Maybe we should think about the organization of files so that key files will be in the root (here, for example, the toast container and the toast function) that are exported or keep them in the "core" directory

import { toastEmitter } from './EventEmitter';

// eslint-disable-next-line import/prefer-default-export
export const toast = ({ message, duration, actions }) => {
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 it would be great to add a little jsDocs documentation for this function

Comment on lines 17 to 22
## Features

- **Customizable Appearance**: Choose the window position for toast.
- **Interactive**: Includes a close button for manual dismissal.
- **Auto-dismiss**: Disappears automatically after a set duration.
- **Hover Interactivity**: Auto-dismiss timer pauses on hover or focus, allowing users to interact with the content.
Copy link
Contributor

Choose a reason for hiding this comment

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

[important] Let's place this description of the functionality of the Toast component in the Behaviors. Now we're repeating ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea

}, []);

return portalDivRef.current ? ReactDOM.createPortal(
<div className="toast-container" style={{ ...positionStyle }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="toast-container" style={{ ...positionStyle }}>
<div className="pgn__toast-container" style={{ ...positionStyle }}>

this.events[event].push(callback);
}

emit(event, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[proposal] In the current implementation, when using several Toasts on a page, we observe their simultaneous calling. After clicking on any of the Toast triggers, we simultaneously call the emitter for all toasts on the page. We need to unify this process and make sure that the trigger executes the showToast event only for the desired Toast.

On the component usage example pages we have Toast in the code example (copy button) and in the usage example itself.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6927788) 93.00% compared to head (21168db) 93.05%.

Files Patch % Lines
src/Toast/BaseToast.jsx 90.90% 2 Missing ⚠️
src/Toast/index.jsx 96.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2959      +/-   ##
==========================================
+ Coverage   93.00%   93.05%   +0.05%     
==========================================
  Files         236      239       +3     
  Lines        4273     4306      +33     
  Branches     1033     1036       +3     
==========================================
+ Hits         3974     4007      +33     
  Misses        295      295              
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants