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

Support the "as" prop to change the wrapper #614

Merged
merged 5 commits into from
May 13, 2020
Merged

Support the "as" prop to change the wrapper #614

merged 5 commits into from
May 13, 2020

Conversation

youknowriad
Copy link
Contributor

In Gutenberg, we have this use-case where we want to control the DOM precisely (to match between the frontend and the editor), and for one of these components we're using Resizable as a wrapper but "div" is not always the right element we want to use there.

We have this particular use-case in a lot of places in our code base and in general we rely on a "as" prop to do so. Other react libraries also use this technique (example Reakit).

I don't know if you're interested by adding the same prop but In this PR I added support for it.

What do you think?

src/index.tsx Outdated
@@ -77,6 +77,7 @@ export type ResizeStartCallback = (
) => void | boolean;

export interface ResizableProps {
as: any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using string | React.ComponentType instead of any here but I faced a typing issue in the "ref" prop as a consequence and I was not able to solve it otherwise.

Copy link
Owner

@bokuweb bokuweb May 13, 2020

Choose a reason for hiding this comment

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

@youknowriad Thanks for your work :)
Could you please specify HTMLElement name like following.

as: 'div' | 'header' | ...|  React.ComponentType<any>;

This is because we need to ban elements that don't have children (i.e. input)
And please replace HTMLDivElement to HTMLElement

Copy link
Owner

@bokuweb bokuweb May 13, 2020

Choose a reason for hiding this comment

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

If you are ok could you please add story and testcase with as?
And please fix README.md too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I added a story and a testcase and replaced HTMLDivElement.
I've used string | React.ComponentType<any>; as a type. is that ok? The list of elements can be long :).

Let me know what's missing thanks.

Copy link
Owner

@bokuweb bokuweb left a comment

Choose a reason for hiding this comment

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

Thanks :) Looks good

@bokuweb bokuweb merged commit 698aba1 into bokuweb:master May 13, 2020
@bokuweb
Copy link
Owner

bokuweb commented May 13, 2020

@youknowriad v6.4.0 published 🎉

@youknowriad
Copy link
Contributor Author

@bokuweb Perfect, thanks for the quick npm release :)

@youknowriad youknowriad deleted the add/as-prop branch May 14, 2020 09:15
oxyc added a commit to oxyc/gutenberg that referenced this pull request Jun 21, 2020
noisysocks pushed a commit to WordPress/gutenberg that referenced this pull request Jun 22, 2020
* Use light block DOM for the Media & Text block

* Update re-resizable dependency

Specifically to support "as" from bokuweb/re-resizable#614
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.

2 participants