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

Add onDecorated property to decorated components #1680

Merged
merged 1 commit into from
Mar 28, 2017

Conversation

chabou
Copy link
Collaborator

@chabou chabou commented Mar 17, 2017

Plugins could be more powerful if we give them a way to retrieve a reference to decorated components.

The only reference that a plugin could have is htermby passing a onTerminal property.
It could use refproperty in decorators but if it is not the first in plugin list, it will receive previous plugin HOC.

With this PR, every plugin's decorate<Component> functions (decorateTerm, decorateTerms, decorateHeader, decorateTab...) could retrieve a reference to instance.

Maybe it is a little dangerous/anti-pattern to expose all instances to plugins, but this is the same logic than onTerminalproperty.

If you are ok with this feature, I'll add website documentation.

@chabou
Copy link
Collaborator Author

chabou commented Mar 17, 2017

@rauchg @matheuss @ppot could you give me your opinion about this feature ?

@rauchg
Copy link
Member

rauchg commented Mar 18, 2017

Wouldn't there be a way for getting the instance yourself when you decorate though?

@chabou
Copy link
Collaborator Author

chabou commented Mar 19, 2017

If you are the only (or the first) plugin, there is a way by passing : refproperty when rendering Term because it is a first level child. But if you are the second plugin, you are not decorating Term itself but the first plugin decorator. If this first plugin doesn't keep a reference to Term instance (using refproperty), I think there is no way to find it (but maybe I'm wrong).

@chabou
Copy link
Collaborator Author

chabou commented Mar 19, 2017

This reason leads to add onTerminal() capability to plugin decorator.
This PR generalize this capability (and could replace it) to all component that could be decorated.

@chabou
Copy link
Collaborator Author

chabou commented Mar 27, 2017

@rauchg I really think that this PR is a good idea 😊

@rauchg rauchg merged commit 7a64c9e into vercel:master Mar 28, 2017
@rauchg
Copy link
Member

rauchg commented Mar 28, 2017

I completely agree. Thanks for your explanation. We need to document this on the website, however.

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