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

[core] Refactor data passing #1249

Merged
merged 9 commits into from
Jan 8, 2025
Merged

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Dec 30, 2024

Part of #1246. Focuses just on anchored popups in this PR

@mui-bot
Copy link

mui-bot commented Dec 30, 2024

Netlify deploy preview

https://deploy-preview-1249--base-ui.netlify.app/

Generated by 🚫 dangerJS against 7ce327b

@atomiks atomiks added the core Infrastructure work going on behind the scenes label Dec 30, 2024
@atomiks atomiks force-pushed the refactor/whole-objects branch from 458923a to ee76ae6 Compare December 30, 2024 09:50
const {
const tooltipRoot = useTooltipRoot({
...props,
defaultOpen,
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of passing onOpenChange, open, and other props explicitly if they are a part of ...props?

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, you're right. Forgot that the linting allows unused variables (as they need to be destructured to be documented)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well not anymore with our latest linting config:

/tmp/base-ui/packages/react/src/popover/root/PopoverRoot.tsx
16:11 error 'defaultOpen' is assigned a value but never used @typescript-eslint/no-unused-vars
16:24 error 'open' is assigned a value but never used @typescript-eslint/no-unused-vars
16:30 error 'onOpenChange' is assigned a value but never used @typescript-eslint/no-unused-vars
...
✖ 12 problems (12 errors, 0 warnings)

@michaldudak
Copy link
Member

Some cases could be simplified even more (less destructuring) if we passed all the props to hooks as they are, without providing default values on a component level. I'm also thinking about reconsidering the context access pattern we use and having the hooks instead of components read context values. This way, we might not need to create new objects just to pass data to hooks as a single parameter.

@atomiks
Copy link
Contributor Author

atomiks commented Jan 3, 2025

Some cases could be simplified even more (less destructuring) if we passed all the props to hooks as they are, without providing default values on a component level.

Are you referring to useAnchorPositioning or other ones too? A lot of the defaults are the same for that one, although a couple are different for each component (like side). Also, doesn't the documentation require the destructuring?

@michaldudak
Copy link
Member

Are you referring to useAnchorPositioning or other ones too?

I mean all our hooks in general (especially the simpler ones). In cases like https://github.com/mui/base-ui/blob/master/packages/react/src/popover/title/PopoverTitle.tsx#L25, we might simplify passing parameters by passing the whole props object to usePopoverTitle and moving usePopoverRootContext to the hook. I see that the Select hooks already read context. The perf gains won't be huge, so it's likely not worth going through the whole codebase, but it could be something to consider in future components.

Also, doesn't the documentation require the destructuring

Yup, we'd have to change the docgen if we wanted to do this.

@atomiks atomiks force-pushed the refactor/whole-objects branch from 2b70f5d to ee76ae6 Compare January 3, 2025 08:41
@atomiks atomiks marked this pull request as ready for review January 3, 2025 08:42
@atomiks atomiks requested a review from colmtuite as a code owner January 3, 2025 08:42
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2025
@atomiks atomiks merged commit bcbceda into mui:master Jan 8, 2025
23 checks passed
@atomiks atomiks deleted the refactor/whole-objects branch January 8, 2025 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants