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

New OverflowContainer wrapper around useOverflow #2152

Open
wants to merge 34 commits into
base: r/overflow-container
Choose a base branch
from

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Jul 18, 2024

Changes

Pulled out of #2120 as PR 3 in the PR stack (PR stack order).

This just adds the OverflowContainer component from the combined PR but continues to use the existing useOverflow algorithm. Additionally, all components now use OverflowContainer instead of useOverflow.

  • OverflowContainer: Takes the items prop (would like be remove in later PRs in the PR stack), passes it to useOverflow(), and put the returned visibleItems along with the items.length in a context.
  • useOverflowContainerContext(): Can be used in components under OverflowContainer to access the context values in OverflowContainer. (#2152 (comment))
  • OverflowContainer.OverflowNode: Wrap overflow content in this component to conditionally render it when overflowing.

I also had to make a small css change to make the iui-breadcrumbs-list stretch to the width of iui-breadcrumbs. This was needed to constrain the width and thus trigger an overflow when the width of the container is smaller than the required width.

Testing

  • All tests continue to pass

Docs

  • Added pertinent JSDocs.
    • Since OverflowContainer's API will change, the JSDocs are currently concise. They will be expanded upon in later PRs in this PR stack.
  • Added changeset

After PR TODOs:

@r100-stack r100-stack self-assigned this Jul 18, 2024
@r100-stack r100-stack marked this pull request as ready for review July 19, 2024 16:07
@r100-stack r100-stack requested review from a team as code owners July 19, 2024 16:07
@r100-stack r100-stack requested review from mayank99 and Ben-Pusey-Bentley and removed request for a team July 19, 2024 16:07

return (
<Box ref={useMergedRefs(ref, containerRef)} {...rest}>
{typeof children === 'function' ? children(visibleCount) : visibleItems}
Copy link
Contributor

Choose a reason for hiding this comment

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

This render prop branching isn't nice, and kinda defeats the purpose of extracting a component (it's basically a hook with extra steps). What is the main reason for calling children as a function?

I'm wondering if it would be better to expose a subcomponent instead.

<OverflowContainer>
  <>{/* regular nodes */}</>

  {/* this is rendered only when overflowing */}
  <OverflowNode></OverflowNode>
</OverflowContainer>

Copy link
Member Author

@r100-stack r100-stack Jul 19, 2024

Choose a reason for hiding this comment

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

What is the main reason for calling children as a function?

I thought of offering the children as a function approach for more customized cases (e.g. Breadcrumbs, MiddleTextTruncation, etc.) that have overflow in the center. With a function, we can get the visibleCount and return custom content based on that.

I'm wondering if it would be better to expose a subcomponent instead.

I will try looking into this approach as an alternative of the children as a function approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I played around with having an OverflowContainer.OverflowNode that only renders its children when it overflows and having an OverflowContainerContext that gives the visibleCount that the OverflowNode can read (https://github.com/iTwin/iTwinUI/pull/2152/files/2758496d61824cb1133b4baeb4155511d2f30033..abd6485a7f996d211cb507ed9585761077a2eb02). This eliminated the need for the children as a function.

I will fix some a minor regression and improve the code a bit more soon as I first wanted to play around and confirm the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the new context API makes sense. I think we should also have a hook called something like useOverflowContext. It would just be a wrapper over useSafeContext(OverflowContainerContext), with the main goal of allowing to easily destructure the return value without needing to import useSafeContext or OverflowContainerContext.

My main concern with the context API is the need to extract child components. While it's generally a good idea to have lots of smaller components rather than one large component, it makes the diff too large in some cases (e.g. TablePaginator).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should also have a hook called something like useOverflowContext.

Added the hook. I called it useOverflowContainerContext since the context is linked with OverflowContainer instead of useOverflow. (a05e8d4 + 99e3536 for one leftover usage in OverflowContainer.tsx)

My main concern with the context API is the need to extract child components. While it's generally a good idea to have lots of smaller components rather than one large component, it makes the diff too large in some cases (e.g. TablePaginator).

That's true. But as of now, I kept the context approach since it solves some of the other problems that we had with the OverflowContainer API from this PR.

Copy link
Contributor

@Ben-Pusey-Bentley Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

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

I don't have any issues with this PR that don't already have comments, so I am approving this.

@r100-stack r100-stack requested a review from a team as a code owner August 29, 2024 16:46
@r100-stack r100-stack requested review from mayank99 and smmr-dn and removed request for a team August 29, 2024 16:46
@r100-stack r100-stack marked this pull request as draft August 29, 2024 16:47
@r100-stack r100-stack marked this pull request as ready for review August 30, 2024 14:28
@r100-stack r100-stack changed the base branch from r/unit-tests-to-e2e to r/overflow-container October 2, 2024 20:41
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