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/scrollbar): add scrollViewProps to enable passing custom props to the ScrollView component #939

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

cheton
Copy link
Member

@cheton cheton commented Oct 16, 2024

Demo: https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-939/components/scrollbar#how-to-integrate-react-virtuoso-with-a-custom-scrollbar

PR Type

Enhancement, Documentation


Description

  • Added a new feature to pass custom props to the ScrollView component via scrollViewProps.
  • Enhanced the Scrollbar component to handle custom event handlers using callEventHandlers.
  • Documented the integration of React Virtuoso with a custom scrollbar, including code examples.
  • Updated the documentation to include the new scrollViewProps in the props table.
  • Added react-virtuoso as a dependency in the project.

Changes walkthrough 📝

Relevant files
Documentation
faq-react-virtuoso.js
Add React Virtuoso integration example with custom scrollbar

packages/react-docs/pages/components/scrollbar/faq-react-virtuoso.js

  • Added a new example demonstrating React Virtuoso integration.
  • Created a CustomScrollbar component for use with Virtuoso.
  • Exported the App component as default.
  • +38/-0   
    index.page.mdx
    Document React Virtuoso integration with custom scrollbar

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

  • Documented integration of React Virtuoso with custom scrollbar.
  • Provided code examples for custom scrollbar implementation.
  • Updated props table to include scrollViewProps.
  • +69/-0   
    Enhancement
    Scrollbar.js
    Add scrollViewProps for custom ScrollView properties         

    packages/react/src/scrollbar/Scrollbar.js

  • Introduced scrollViewProps to pass custom props to ScrollView.
  • Utilized callEventHandlers for event handling.
  • Enhanced getScrollViewProps to include scrollViewProps.
  • +7/-4     
    Dependencies
    package.json
    Add react-virtuoso dependency                                                       

    packages/react-docs/package.json

    • Added react-virtuoso as a new dependency.
    +1/-0     

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

    Copy link

    codesandbox bot commented Oct 16, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    changeset-bot bot commented Oct 16, 2024

    🦋 Changeset detected

    Latest commit: 5831187

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 1 package
    Name Type
    @tonic-ui/react Minor

    Not sure what this means? Click here to learn what changesets are.

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Performance Consideration
    The CustomScrollbar component is created for each Virtuoso instance. Consider memoizing this component to prevent unnecessary re-renders.

    Prop Types
    The CustomScrollbar component is missing prop type definitions. Consider adding PropTypes or TypeScript types for better documentation and type safety.

    Copy link

    codesandbox-ci bot commented Oct 16, 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 16, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 77.81%. Comparing base (7b994c5) to head (5831187).
    Report is 1 commits behind head on v2.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##               v2     #939      +/-   ##
    ==========================================
    + Coverage   77.80%   77.81%   +0.01%     
    ==========================================
      Files         403      403              
      Lines        6636     6636              
    ==========================================
    + Hits         5163     5164       +1     
    + Misses       1473     1472       -1     

    ☔ 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 16, 2024

    Tonic UI Demo

    On 2024-10-17 03:20:28 +0000, PR #939 (5831187) was successfully deployed. You can view it at the following link:
    https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-939/

    @cheton cheton changed the title docs(scrollbar): add React Virtuoso integration example feat(react/scrollbar): introduce scrollViewProps to allow custom props to be passed to the internal ScrollView component Oct 17, 2024
    @cheton cheton changed the title feat(react/scrollbar): introduce scrollViewProps to allow custom props to be passed to the internal ScrollView component feat(react/scrollbar): add scrollViewProps to enable passing custom props to the ScrollView component Oct 17, 2024
    @cheton
    Copy link
    Member Author

    cheton commented Oct 17, 2024

    /describe

    Copy link
    Contributor

    PR Description updated to latest commit (3eaafb6)

    @cheton
    Copy link
    Member Author

    cheton commented Oct 17, 2024

    /improve

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 17, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 3eaafb6

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Implement error handling for the Virtuoso component to improve robustness

    Consider adding error handling or a fallback UI in case the Virtuoso component fails
    to render or load properly. This can improve the user experience in case of
    unexpected issues.

    packages/react-docs/pages/components/scrollbar/faq-react-virtuoso.js [21-36]

     const App = () => {
       return (
    -    <Virtuoso
    -      components={{
    -        Scroller: CustomScrollbar,
    -      }}
    -      style={{
    -        height: 400,
    -      }}
    -      totalCount={10000}
    -      itemContent={(index) => (
    -        <Box>Item {index}</Box>
    -      )}
    -    />
    +    <ErrorBoundary fallback={<div>Error loading list</div>}>
    +      <Virtuoso
    +        components={{
    +          Scroller: CustomScrollbar,
    +        }}
    +        style={{
    +          height: 400,
    +        }}
    +        totalCount={10000}
    +        itemContent={(index) => (
    +          <Box>Item {index}</Box>
    +        )}
    +      />
    +    </ErrorBoundary>
       );
     };
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why:

    7
    Use nullish coalescing for more precise default value assignment

    Consider using the nullish coalescing operator (??) instead of the logical OR
    operator (||) when setting default values for overflowX and overflowY. This will
    allow explicit false values to be passed through.

    packages/react/src/scrollbar/Scrollbar.js [35-45]

    -const overflowX = overflowXProp || overflow;
    -const overflowY = overflowYProp || overflow;
    +const overflowX = overflowXProp ?? overflow;
    +const overflowY = overflowYProp ?? overflow;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why:

    7
    Best practice
    Destructure event handlers from scrollViewPropsProp for improved readability and error prevention

    Consider destructuring scrollViewPropsProp to extract onScroll, onMouseEnter, and
    onMouseLeave separately. This can improve code readability and potentially catch any
    undefined property access errors earlier.

    packages/react/src/scrollbar/Scrollbar.js [555-557]

    -onScroll: callEventHandlers(scrollViewPropsProp?.onScroll, handleScrollViewScroll),
    -onMouseEnter: callEventHandlers(scrollViewPropsProp?.onMouseEnter, handleScrollViewMouseEnter),
    -onMouseLeave: callEventHandlers(scrollViewPropsProp?.onMouseLeave, handleScrollViewMouseLeave),
    +const { onScroll, onMouseEnter, onMouseLeave } = scrollViewPropsProp || {};
    +...
    +onScroll: callEventHandlers(onScroll, handleScrollViewScroll),
    +onMouseEnter: callEventHandlers(onMouseEnter, handleScrollViewMouseEnter),
    +onMouseLeave: callEventHandlers(onMouseLeave, handleScrollViewMouseLeave),
    Suggestion importance[1-10]: 7

    Why:

    7

    Previous suggestions

    Suggestions up to commit d10a663
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use "auto" instead of "scroll" for overflowY to show scrollbar only when needed

    Consider setting overflowY to "auto" instead of "scroll" in the Scrollbar component.
    This allows the browser to show the scrollbar only when necessary, improving the
    user interface when there's not enough content to scroll.

    packages/react-docs/pages/components/scrollbar/faq-react-virtuoso.js [7-9]

     <Scrollbar
    -  overflowY="scroll"
    +  overflowY="auto"
       scrollViewRef={ref}
     >
    Suggestion importance[1-10]: 7

    Why:

    7
    Possible issue
    Add error handling for null or undefined ref to prevent potential runtime errors

    Add error handling for the case where the ref might be null or undefined. This can
    prevent potential runtime errors and improve the robustness of the component.

    packages/react-docs/pages/components/scrollbar/faq-react-virtuoso.js [5-9]

     const CustomScrollbar = forwardRef((props, ref) => {
    +  if (!ref) {
    +    console.warn('CustomScrollbar: ref is null or undefined');
    +    return null;
    +  }
       return (
         <Scrollbar
           overflowY="scroll"
           scrollViewRef={ref}
         >
    Suggestion importance[1-10]: 7

    Why:

    7
    Performance
    Memoize the CustomScrollbar component to optimize performance by preventing unnecessary re-renders

    Consider memoizing the CustomScrollbar component using React.memo() to optimize
    performance by preventing unnecessary re-renders when the props haven't changed.

    packages/react-docs/pages/components/scrollbar/faq-react-virtuoso.js [5-9]

    -const CustomScrollbar = forwardRef((props, ref) => {
    +const CustomScrollbar = React.memo(forwardRef((props, ref) => {
       return (
         <Scrollbar
           overflowY="scroll"
           scrollViewRef={ref}
         >
    +}));
    Suggestion importance[1-10]: 7

    Why:

    7

    @cheton cheton requested a review from a team October 17, 2024 03:17
    @cheton cheton merged commit 43d3615 into v2 Oct 17, 2024
    7 checks passed
    @cheton cheton deleted the tonic-ui-938 branch October 17, 2024 03:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants