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

[Finishes #38] Add override default fallback component example #44

Merged
merged 1 commit into from
Aug 31, 2017

Conversation

thoragio
Copy link
Contributor

@kennylavender I think this nails it succinctly but let me know what you think (and please confirm I got my composition right!).

@kennylavender
Copy link
Contributor

kennylavender commented Aug 31, 2017

Hmm, I hadn't thought about using it this way, I don't know that I would import the 404 feature to override it with a coming soon, but I can't decide if I like it or not haha. Nice. It does also explain the ability to override a default in a nice a short way... hmmm

So I started writing up a new issue last night for this next example, but forgot to finish it, what I was wanting the second example to show

  • A page with many features
  • A few of those will be wrapped with configureFeature to conditionally render
  • There would be a default fallback used for some of the features
  • Then one of the features would use an override

But I do like what you have done here, I am not sure what I like better, maybe go with what you have here for now and we can add another example like I put above later if needed?

@kennylavender
Copy link
Contributor

I am thinking I like your version better, if you agree, lets go with it

@kennylavender
Copy link
Contributor

I am glad I did not guide you to my thoughts for this with the issue last night ;)

@kennylavender
Copy link
Contributor

I think this should be ready to pull into other projects after this, if we have not done it already

@thoragio
Copy link
Contributor Author

To be honest, I could not figure out another way to do it, so I documented the only way I could see how. :-)

Maybe we could change the name of the component to something else other than 404, but I am not sure that would make any real difference.

I'm fine with it as it is unless you have any other ideas. I'll merge this one for now, but feel free to add any changes you come up with later.

@thoragio thoragio merged commit 6815e0d into master Aug 31, 2017
@thoragio thoragio deleted the examples branch August 31, 2017 14:42
Copy link
Contributor

@ericelliott ericelliott left a comment

Choose a reason for hiding this comment

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

Clarifying real-life use-cases.


const help404 = feature404('help-chat');

export default help404(HelpChatComponent, ComingSoonComponent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ComingSoonComponent should be called HelpFallbackComponent or something similar, to make it more clear.

In real use, I usually render entire pages as 404, or render nothing -- both of which users could configure as reusable fallback components that would work everywhere (like featureOr404(), featureOrNothing(), etc... I think the case where you render ComingSoon would be extremely rare. What you might render instead would be an older version of a component, like help404(HelpChatComponent, OldHelpComponent);

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.

None yet

3 participants