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

[ScrollArea] Viewport fixes #2945

Merged
merged 10 commits into from
Aug 11, 2024
Merged

[ScrollArea] Viewport fixes #2945

merged 10 commits into from
Aug 11, 2024

Conversation

vladmoroz
Copy link
Contributor

  • Fix asChild prop not working as expected on the Viewport part
  • Rework the inner content element styles on the Viewport part
    • Replace display: table with flex styles on the parent that allow the viewport to fill container height (based on the Radix Themes implementation)
    • Replace most related inline styles with 0,0,0 specificity styles for easier overrides via [data-radix-scroll-area-content]
    • Fix text-overflow: ellipsis not working when used with a vertical scrollbar

Closes #926 #1666

Comment on lines +1038 to +1057
/**
* This is a helper function that is used when a component supports `asChild`
* using the `Slot` component but its implementation contains nested DOM elements.
*
* Using it ensures if a consumer uses the `asChild` prop, the elements are in
* correct order in the DOM, adopting the intended consumer `children`.
*/
function getSubtree(
options: { asChild: boolean | undefined; children: React.ReactNode },
content: React.ReactNode | ((children: React.ReactNode) => React.ReactNode)
) {
const { asChild, children } = options;
if (!asChild) return typeof content === 'function' ? content(children) : content;

const firstChild = React.Children.only(children) as React.ReactElement;
return React.cloneElement(firstChild, {
children: typeof content === 'function' ? content(firstChild.props.children) : content,
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We worked this out in radix-ui/themes#311

@mboudreau
Copy link

Any progress on this? I just hit this issue today...

@fkysly
Copy link

fkysly commented Jul 27, 2024

Any progress on this too...

@Crisis2010
Copy link

Same here, have tmp fix, but need proper

@vladmoroz vladmoroz merged commit f6d70b3 into main Aug 11, 2024
5 checks passed
@vladmoroz vladmoroz deleted the vlad/scroll-area-fixes branch August 11, 2024 08:51
@lazakrisz
Copy link

thank you so much folks 🙏

@kognise
Copy link

kognise commented Nov 7, 2024

FYI this caused a breaking regression for us. width: min-content on the inner container caused horizontal layouts inside vertical scroll areas to not be full width. For now we are doing:

.rt-ScrollAreaViewport>* {
  width: 100%;
}

but it doesn't seem very correct? Not entirely sure.

@TimChild
Copy link

I have the same issue as @kognise

chaance added a commit that referenced this pull request Nov 12, 2024
chaance added a commit that referenced this pull request Nov 12, 2024
@chaance
Copy link
Member

chaance commented Nov 12, 2024

Reverted in 1.2.1 to fix the regression. Will revisit this at a later date.

@Georgegriff
Copy link

That's unfortunate, the removal of display table in this pr fixed other things, such as being able to properly use flexbox to dynamically size the scroll height, was relying on it. hopefully there's a way of bringing it back

@chaance
Copy link
Member

chaance commented Nov 13, 2024

@Georgegriff I know. I'm planning to come back to this but I think the splash radius here for breaking apps is a little too big to justify in a patch release. We'll either re-release it in a major or adjust it to avoid breakage.

Feel free to pin to 1.2.0 in the mean time.

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.

[ScrollArea] Content with ellipsis CSS bursts out of container.
9 participants