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

Fix partial TermGroup decoration #1705

Merged
merged 2 commits into from
Mar 27, 2017
Merged

Conversation

chabou
Copy link
Collaborator

@chabou chabou commented Mar 26, 2017

First TermGroup of each window is decorated here

But when a TermGroup add a sub-TermGroup in a SplitPane, it is not decorated.

This PR fix this.

@chabou
Copy link
Collaborator Author

chabou commented Mar 26, 2017

Warning!
This PR could break plugins which assume that decorateTermGroup and getTermGroupProps are called only for each RootTermGroup.
It could be better to not merge this PR and warn somewhere in docs than only main TermGroup is decorated. I think that @ppot rework will fix this partial decoration.

@chabou
Copy link
Collaborator Author

chabou commented Mar 26, 2017

Maybe hyper-pane is the only plugin which assumes that 🤣
In this case, I can fix it and we could merge this PR

@rauchg
Copy link
Member

rauchg commented Mar 27, 2017

@chabou, this is the behavior I'd expect. So let's take our chances!

@rauchg rauchg merged commit 6f2a77f into vercel:master Mar 27, 2017
@chabou
Copy link
Collaborator Author

chabou commented Mar 27, 2017

@rauchg I've scripted a check for all plugins listed in https://github.com/bnb/awesome-hyper

Only 8 plugins use getTermGroupProps. 7 use this hook only to pass down props without any action. So they'll work without problem. The only one which use some logic and will not work is mine: hyper-pane. I'll fix it very soon.

No plugin use decorateTermGroup.

We can conclude that this PR will not break anything 🎉

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