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

[Popover] Fix missing type export getOffsetTop & getOffsetLeft #32959

Merged
merged 1 commit into from Jun 8, 2022

Conversation

rart
Copy link
Contributor

@rart rart commented May 31, 2022

Wondering if there's a reason why getOffsetTop and getOffsetLeft aren't exported from the index? They're actually exported from the main Popover.js file but not from the index or typings. Almost seems like a miss.

We've found use for these when programmatically supplying a position to the Popover component (i.e. anchorReference: 'anchorPosition' and anchorPostion: { top, left }).

To use them, we have to ts-ignore the line due to the lack of typings and import from the 4th level which is meant to be private.

// @ts-ignore
import { getOffsetLeft, getOffsetTop } from '@mui/material/Popover';

It'd be nice to have them public

import { getOffsetLeft, getOffsetTop } from '@mui/material/Popover';

@mui-bot
Copy link

mui-bot commented May 31, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 2df4bc7

@rart rart force-pushed the export-popover-offset-getters branch from 622de21 to cf15e80 Compare May 31, 2022 13:28
@rart rart force-pushed the export-popover-offset-getters branch from cf15e80 to 2df4bc7 Compare May 31, 2022 13:38
@mnajdova mnajdova added new feature New feature or request component: Popover The React component. labels Jun 8, 2022
@mnajdova mnajdova merged commit 3879455 into mui:master Jun 8, 2022
@oliviertassinari oliviertassinari added typescript bug 🐛 Something doesn't work labels Jun 8, 2022
@oliviertassinari oliviertassinari changed the title [Popover] Export getOffsetTop & getOffsetLeft from Popover's index and add typings [Popover] Fix missing type export getOffsetTop & getOffsetLeft from Popover Jun 8, 2022
@oliviertassinari oliviertassinari changed the title [Popover] Fix missing type export getOffsetTop & getOffsetLeft from Popover [Popover] Fix missing type export getOffsetTop & getOffsetLeft Jun 8, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 8, 2022

To be fair, there is a high chance that we will rewrite Popover to use the positioning logic of Popper: #17636. So these methods will likely go deprecated. Though, it's great to have it in the types, so we can add a clear @deprecated mention in the future!

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 8, 2022

Wondering if there's a reason why getOffsetTop and getOffsetLeft aren't exported from the index?

AFAIK, the exports were meant for maintainers, so we could write unit tests. I mean, I don't see the use case for developers.

@rart
Copy link
Contributor Author

rart commented Jun 8, 2022

I don't see the use case for developers.

@oliviertassinari as briefly explained on the PR description, we found use for them when programmatically calculating the Popover's position (when using anchorReference: 'anchorPosition') instead of supplying the actual element for the Popover to internally figure it out. Not sure the full details are of interest, happy to provide further detail if so, but in essence: the Popover props come from Redux and since having non-serializables on the state is a bad practice, we store the top & left and the getOffset* functions were useful to compute these. They saved us from duplicating pretty much same logic. 🤷🏻‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Popover The React component. new feature New feature or request typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants