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

Create next example #201

Closed
wants to merge 13 commits into from
Closed

Create next example #201

wants to merge 13 commits into from

Conversation

kennylavender
Copy link
Contributor

@kennylavender kennylavender commented Mar 9, 2019

closes #202

Create an example for next

@kennylavender kennylavender self-assigned this Mar 9, 2019
@kennylavender
Copy link
Contributor Author

kennylavender commented Mar 9, 2019

There seems to be an issue when you go to /recently-active-users via the client side router, it doesn't render the 404 error, it renders the next unexpected error. It does this because we are not passing the component a statusCode on the client side. Il need to look into how best to provide the statusCode or maybe we can just default to 404.

@kennylavender
Copy link
Contributor Author

WIP, this needs more work, don't bother reviewing atm.

@kennylavender
Copy link
Contributor Author

Ready for review. I think it can be improved in a few ways, but this is a good starting point.

() =>
<li>
{/*
Eventually we may add a function that applies currently activated
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have access to initial features from any component under the features provider in the component tree?

Copy link
Contributor Author

@kennylavender kennylavender Mar 9, 2019

Choose a reason for hiding this comment

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

We do, I haven't yet created an easy to use function, component or hook to apply them to a given path. #174

@ericelliott
Copy link
Contributor

It looks like there's a whole lot of ad-hoc plumbing in the example that should probably be part of the library. Does every user need to jump through all these hoops and create a dozen different files just to conditionally display a feature page or 404 page?

If so, why?

@ericelliott
Copy link
Contributor

I'd like to see a minimal example of this in action using about 1/10th as much code. ;)

@@ -0,0 +1,15 @@
# Next Example
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example needs much better documentation. What is the purpose of all these different files? Why did it take 20 different files to demonstrate this feature? Is this it reasonable for a user of this library to see all these files and absorb what each one is doing and figure out to implement the feature in their application within 20-30 minutes?

It should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next requires a bunch more work and customization due to having to use getInitialProps.

We could build a library to supply these next specific HOC's, next-react-feature-toggles. I guess they could come from this library too, but I was avoiding making this specific to next at the start.

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.

See comments.

@kennylavender
Copy link
Contributor Author

kennylavender commented Mar 9, 2019

I'd like to see a minimal example of this in action using about 1/10th as much code. ;)

I agree, that was my goal when I started, but I realized writing this out like a real next app helped me understand the strengths and weaknesses of the current API. I was thinking code like this might make good code for integration tests, we could run integration tests against it with cypress.io. Then we could have simpler example code. What do you think?

Edit: Would make sense to me especially if we create next specific library or provide the next specifics from this library.

@ericelliott
Copy link
Contributor

Check out how RITEway separately exports a React tool to help React users without requiring all users to install and use React.

As a user, I think I'd prefer it to be colocated with this same library, but perhaps exported separately.

@kennylavender
Copy link
Contributor Author

Closing, will go in new smaller example direction after creating new next specific code.

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.

Create next example
2 participants