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

Document getAnchorRect prop for Popover component. #17709

Merged
merged 3 commits into from
Apr 9, 2020
Merged

Document getAnchorRect prop for Popover component. #17709

merged 3 commits into from
Apr 9, 2020

Conversation

desaiuditd
Copy link
Member

Description

Add missing documentation for this hidden gem-like prop for Popover component.

Types of changes

Documentation change. (Non-breaking)

@ellatrix
Copy link
Member

ellatrix commented Oct 2, 2019

To be honest, I'm not sure if getAnchorRect is the best API we can provide. I'll have a look into this soon. In the meantime, I'm also not sure if we should document it, maybe others have an opinion on this.

@youknowriad
Copy link
Contributor

I was thinking about this too and I agree that we can have a more friendly prop, "position stategy" or something like that and it could have values like "parent" (default), "element" (pass a ref there somehow)...

That said, that prop still need to be supported anyway, so I won't be against documenting it now since it's not deprecated.

@ellatrix
Copy link
Member

ellatrix commented Oct 2, 2019

Sounds good. Yeah, I was thinking if we could pass reference nodes directly to Popover... And allow a buffer to be set, an option to ignore padding, etc. Would simplify a lot of use cases.

@desaiuditd
Copy link
Member Author

To be honest, I'm not sure if getAnchorRect is the best API we can provide. I'll have a look into this soon.

I wasn't aware this is going to be changed. (cc: @aprea) At my work, I stumbled upon this and I had to use it in one of the use-case that I wanted to support. So I used it and thought that this should be documented. It was hard enough for me to understand it by reading the code, and not many people would know about it. So better to document it. Happy to wait until it's matured and close this or leave this open for comments/review.

@desaiuditd
Copy link
Member Author

@ellatrix @youknowriad Can this be reviewed and merged, if all okay?

@desaiuditd desaiuditd added the [Type] Developer Documentation Documentation for developers label Apr 6, 2020
@youknowriad youknowriad merged commit acafff9 into WordPress:master Apr 9, 2020
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 9, 2020
@ellatrix
Copy link
Member

ellatrix commented Apr 9, 2020

@youknowriad @peterwilsoncc I'm not sure if we should be documenting this. Currently we don't use this prop at all anymore anywhere in Gutenberg. Instead we use (and should use) anchorRef.

@youknowriad
Copy link
Contributor

Documenting it or not doesn't change the fact that it's a public API that we need to support. If you think it's a deprecated API, why not mark it explicitly (trigger a deprecated call?)

@desaiuditd desaiuditd deleted the add/documentation-for-getanchorrect-prop-for-popover-component branch May 10, 2020 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants