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

feat(react/toast): enhance inline toast transition management #940

Merged
merged 19 commits into from
Nov 8, 2024

Conversation

cheton
Copy link
Member

@cheton cheton commented Oct 29, 2024

Demo: https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-940/components/toast#toast-on-modal-and-drawer

You can create your customized InlineToastContainer component for modals or drawers with the following example:

const InlineToastContainer = (props) => (
  <Flex
    flexDirection="column"
    alignItems="center"
    position="absolute"
    top="12x"
    left="50%"
    transform="translateX(-50%)"
    width="max-content"
    maxWidth="80%" // up to 80% of the drawer width
    zIndex="toast"
    {...props}
  />
);

Description

  • Introduced a new ToastGroup component to manage toast notifications without ToastManager.
  • Refactored drawer-toast.js and modal-toast.js to utilize ToastGroup for toast management.
  • Updated tests to include ToastGroup in expected exports.
  • Enhanced documentation to provide examples and usage details for ToastGroup.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
drawer-toast.js
Refactor drawer toast to use ToastGroup                                   

packages/react-docs/pages/components/toast/drawer-toast.js

  • Replaced ToastController and ToastTransition with ToastGroup.
  • Updated toast notification logic to use ToastGroup.
  • Simplified toast content creation and management.
  • +88/-105
    modal-toast.js
    Refactor modal toast to use ToastGroup                                     

    packages/react-docs/pages/components/toast/modal-toast.js

  • Replaced ToastController and ToastTransition with ToastGroup.
  • Updated toast notification logic to use ToastGroup.
  • Simplified toast content creation and management.
  • +88/-105
    ToastGroup.js
    Implement new ToastGroup component for toast management   

    packages/react/src/toast/ToastGroup.js

  • Introduced new ToastGroup component.
  • Handles rendering and managing multiple toasts.
  • Utilizes TransitionGroup for animations.
  • +69/-0   
    ToastManager.js
    Refactor ToastManager to use ToastGroup                                   

    packages/react/src/toast/ToastManager.js

  • Replaced TransitionGroup logic with ToastGroup.
  • Simplified toast creation and management.
  • Added ToastContainerComponent and ToastContainerProps.
  • +51/-96 
    index.js
    Export ToastGroup in toast index                                                 

    packages/react/src/toast/index.js

    • Exported new ToastGroup component.
    +2/-0     
    Tests
    index.test.js
    Add ToastGroup to expected exports in tests                           

    packages/react/tests/index.test.js

    • Added ToastGroup to the list of expected exports.
    +1/-0     
    Documentation
    index.page.mdx
    Update toast documentation with ToastGroup usage                 

    packages/react-docs/pages/components/toast/index.page.mdx

  • Updated documentation to include ToastGroup.
  • Provided examples for using ToastGroup.
  • Removed ToastController references.
  • +85/-6   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    codesandbox bot commented Oct 29, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    changeset-bot bot commented Oct 29, 2024

    ⚠️ No Changeset found

    Latest commit: f999222

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link

    codesandbox-ci bot commented Oct 29, 2024

    This pull request is automatically built and testable in CodeSandbox.

    To see build info of the built libraries, click here or the icon next to each commit SHA.

    Copy link

    codecov bot commented Oct 29, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 78.34%. Comparing base (2dff356) to head (ac26a80).
    Report is 1 commits behind head on v2.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##               v2     #940      +/-   ##
    ==========================================
    + Coverage   77.81%   78.34%   +0.52%     
    ==========================================
      Files         403      405       +2     
      Lines        6636     6640       +4     
    ==========================================
    + Hits         5164     5202      +38     
    + Misses       1472     1438      -34     

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

    @trendmicro-frontend-bot
    Copy link
    Contributor

    trendmicro-frontend-bot commented Oct 29, 2024

    Tonic UI Demo

    On 2024-11-08 10:12:50 +0000, PR #940 (ac26a80) was successfully deployed. You can view it at the following link:
    https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-940/

    @cheton cheton marked this pull request as ready for review October 30, 2024 05:07
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    New Component
    Verify the implementation of the new ToastGroup component, ensuring it correctly manages toast notifications without ToastManager.

    Refactoring
    Review the changes in ToastManager, particularly the removal of createToast method and modifications to the notify method.

    API Changes
    Check the updated implementation using ToastGroup instead of ToastController and ToastTransition.

    @cheton cheton changed the title feat: enable toast management through ToastGroup without ToastManager feat: enable toast transition management through ToastTransitionGroup without ToastManager Oct 30, 2024
    @cheton cheton changed the title feat: enable toast transition management through ToastTransitionGroup without ToastManager feat: enable toast transition management through ToastTransitionController without ToastManager Nov 4, 2024
    @cheton cheton changed the title feat: enable toast transition management through ToastTransitionController without ToastManager feat: add ToastTransitionController to allow support for inline toast transition management Nov 4, 2024
    @cheton
    Copy link
    Member Author

    cheton commented Nov 5, 2024

    /improve

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 5, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 81debab

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add test cases for newly introduced components to ensure proper functionality

    Consider adding test cases for the newly added ToastTransitionController and
    ToastTransitionGroup components to ensure their functionality.

    packages/react/tests/index.test.js [245-246]

     'ToastManager',
     'ToastMessage',
     'ToastTransition',
     'ToastTransitionController',
     'ToastTransitionGroup',
     'useToastManager',
     
    +// Add test cases for new components
    +test('ToastTransitionController renders correctly', () => {
    +  // Add test implementation
    +});
    +
    +test('ToastTransitionGroup renders correctly', () => {
    +  // Add test implementation
    +});
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding test cases for new components is crucial for ensuring their functionality and preventing regressions. This suggestion significantly improves the overall quality and reliability of the codebase.

    8
    Use optional chaining and null checks to improve robustness when accessing nested properties

    Consider using optional chaining (?.) for accessing nested properties to avoid
    potential errors if the object is undefined.

    packages/react/src/toast/ToastManager.js [117-122]

    -const placement = find(id)?.placement;
    +const toast = find(id);
    +const placement = toast?.placement;
     const index = findIndex(id);
     
    -if (!placement || index === -1) {
    +if (!toast || !placement || index === -1) {
       return false;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code robustness by adding an additional null check for the toast object and using optional chaining. It helps prevent potential errors when accessing nested properties of undefined objects.

    7
    ✅ Reorder spread operators to allow overriding of default transition properties
    Suggestion Impact:The spread operator for TransitionProps was placed before other props, allowing for overriding of default transition properties.

    code diff:

    -      {...TransitionProps}
    -      {...rest}

    Consider using the spread operator for TransitionProps after other props to allow
    overriding of default transition properties.

    packages/react/src/toast/ToastTransitionController.js [18-23]

     <TransitionComponent
       in={true}
       unmountOnExit
    +  {...rest}
       {...TransitionProps}
    -  {...rest}
     >
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Reordering the spread operators allows for more flexible prop overriding, which can be useful for customizing transition properties. However, the impact is moderate as it depends on specific use cases.

    6
    Best practice
    Add prop type validation to improve component reliability and developer experience

    Consider adding prop type validation using PropTypes or TypeScript to improve
    component reliability and developer experience.

    packages/react/src/toast/ToastTransitionController.js [7-15]

    +import PropTypes from 'prop-types';
    +
     const ToastTransitionController = (inProps) => {
       const {
         TransitionComponent = ToastTransition,
         TransitionProps,
         children,
         duration: durationProp = null,
         onClose: onCloseProp,
         ...rest
       } = useDefaultProps({ props: inProps, name: 'ToastTransitionController' });
     
    +// Component implementation...
    +};
    +
    +ToastTransitionController.propTypes = {
    +  TransitionComponent: PropTypes.elementType,
    +  TransitionProps: PropTypes.object,
    +  children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),
    +  duration: PropTypes.number,
    +  onClose: PropTypes.func,
    +};
    +
    Suggestion importance[1-10]: 7

    Why: Adding prop type validation enhances code reliability and improves the developer experience by catching potential errors early and providing better documentation for component usage.

    7

    Previous suggestions

    ✅ Suggestions up to commit f53549d
    CategorySuggestion                                                                                                                                    Score
    Performance
    Use a Map instead of an object for more efficient toast management

    Consider using a more efficient data structure, such as a Map, for managing toasts
    instead of an object, especially if you expect to have a large number of toasts or
    frequent updates.

    packages/react/src/toast/ToastManager.js [51-56]

     const [state, setState] = useState(() => (
    -  placements.reduce((acc, placement) => {
    -    acc[placement] = [];
    -    return acc;
    -  }, {})
    +  new Map(placements.map(placement => [placement, []]))
     ));
    Suggestion importance[1-10]: 8

    Why: Using a Map for managing toasts can enhance performance, especially with a large number of toasts or frequent updates. This suggestion addresses potential efficiency improvements in the code.

    8
    Best practice
    Add prop type validation for the ToastGroup component

    Consider adding a prop type validation for the toasts prop to ensure it receives an
    array of objects with the expected structure.

    packages/react/src/toast/ToastGroup.js [12-18]

    +import PropTypes from 'prop-types';
    +
     const ToastGroup = (inProps) => {
       const {
         TransitionComponent = ToastTransition,
         TransitionProps,
         toasts,
         onClose: onCloseProp,
       } = useDefaultProps({ props: inProps, name: 'ToastGroup' });
     
    +ToastGroup.propTypes = {
    +  toasts: PropTypes.arrayOf(
    +    PropTypes.shape({
    +      id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
    +      content: PropTypes.func.isRequired,
    +      duration: PropTypes.number,
    +    })
    +  ).isRequired,
    +  onClose: PropTypes.func.isRequired,
    +};
    +
    Suggestion importance[1-10]: 7

    Why: Adding prop type validation is a best practice that ensures the toasts prop receives the correct structure, improving robustness and error handling. This is beneficial for maintainability and debugging.

    7
    Enhancement
    ✅ Extract toast message content into a separate function for better organization
    Suggestion Impact:The toast message content was extracted into a separate constant within the handleClickAddToastByAppearance function, improving code organization.

    code diff:

    -  const handleClickAddToastByAppearance = (appearance) => (event) => {
    -    const content = {
    -      success: (
    -        <>
    -          <Text>This is a success message.</Text>
    -          <Text>The toast will be automatically dismissed after 5 seconds.</Text>
    -        </>
    -      ),
    -      info: (
    -        <>
    -          <Text>This is an info message.</Text>
    -          <Text>The toast will remain visible until the user dismisses it.</Text>
    -        </>
    -      ),
    -      warning: (
    -        <>
    -          <Text>This is a warning message.</Text>
    -          <Text>The toast will remain visible until the user dismisses it.</Text>
    -        </>
    -      ),
    -      error: (
    -        <>
    -          <Text>This is an error message.</Text>
    -          <Text>The toast will remain visible until the user dismisses it.</Text>
    -        </>
    -      ),
    -    }[appearance];

    Consider extracting the toast message content into a separate constant or function
    to improve code readability and maintainability.

    packages/react-docs/pages/components/toast/drawer-toast.js [75-100]

    -const toastMessage = {
    -  success: (
    -    <>
    -      <Text>This is a success message.</Text>
    -      <Text>The toast will be automatically dismissed after 5 seconds.</Text>
    -    </>
    -  ),
    -  info: (
    -    <>
    -      <Text>This is an info message.</Text>
    -      <Text>The toast will be automatically dismissed after 5 seconds.</Text>
    -    </>
    -  ),
    -  warning: (
    -    <>
    -      <Text>This is a warning message.</Text>
    -      <Text>The toast will remain visible until the user dismisses it.</Text>
    -    </>
    -  ),
    -}[appearance];
    +const getToastMessage = (appearance) => {
    +  const messages = {
    +    success: (
    +      <>
    +        <Text>This is a success message.</Text>
    +        <Text>The toast will be automatically dismissed after 5 seconds.</Text>
    +      </>
    +    ),
    +    info: (
    +      <>
    +        <Text>This is an info message.</Text>
    +        <Text>The toast will be automatically dismissed after 5 seconds.</Text>
    +      </>
    +    ),
    +    warning: (
    +      <>
    +        <Text>This is a warning message.</Text>
    +        <Text>The toast will remain visible until the user dismisses it.</Text>
    +      </>
    +    ),
    +  };
    +  return messages[appearance] || null;
    +};
     
    +const toastMessage = getToastMessage(appearance);
    +
    Suggestion importance[1-10]: 6

    Why: Extracting the toast message content into a separate function enhances code readability and maintainability by organizing the code better. This is a good practice, but the impact is not critical.

    6
    Rename the component to better reflect its specific use case

    Consider using a more descriptive name for the InlineToastContainer component, such
    as DrawerToastContainer, to better reflect its specific use in the drawer context.

    packages/react-docs/pages/components/toast/drawer-toast.js [24-37]

    -const InlineToastContainer = (props) => (
    +const DrawerToastContainer = (props) => (
       <Flex
         flexDirection="column"
         alignItems="center"
         position="absolute"
         top="12x"
         left="50%"
         transform="translateX(-50%)"
         width="max-content"
         maxWidth="80%" // up to 80% of the drawer width
         zIndex="toast"
         {...props}
       />
     );
    Suggestion importance[1-10]: 5

    Why: The suggestion to rename InlineToastContainer to DrawerToastContainer improves the clarity of the component's purpose in the drawer context. However, the impact is moderate as it is primarily a naming convention change.

    5

    @cheton cheton changed the title feat: add ToastTransitionController to allow support for inline toast transition management feat(Toast): enhance inline toast transition management Nov 7, 2024
    @cheton cheton changed the title feat(Toast): enhance inline toast transition management feat(react/toast): enhance inline toast transition management Nov 7, 2024
    @cheton cheton merged commit 56d92cd into v2 Nov 8, 2024
    7 checks passed
    @cheton cheton deleted the tonic-ui-933 branch November 8, 2024 10:24
    cheton added a commit that referenced this pull request Nov 11, 2024
    cheton added a commit that referenced this pull request Nov 11, 2024
    * chore: add changeset for PR #940
    
    * docs: clean up theme configuration
    
    * revert: remove `ToastContainerComponent` and `ToastContainerProps` props
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    2 participants