-
Notifications
You must be signed in to change notification settings - Fork 366
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
feat(Preview): add startRoute prop to override Provider default #868
Conversation
@SSHari is attempting to deploy a commit to the CodeSandbox Team on Vercel. A member of the Team first needs to authorize it. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b5db453:
|
Hey, @SSHari. Thanks for opening this issue. What do you think about this approach here? #873 It seems simpler, and it doesn't overcomplicate the client creation. But, being honest, your approach is more agnostic than the one I tried. I didn't like that I needed to add custom logic on each client to make it work. Let's give it a think. |
@danilowoz I think the approach you're suggesting is reasonable. While you have custom logic in each place, the changes are colocated with the general iframe logic for each client. It's probably easier to understand what's happening in your version for other people who are reading the code. As you said, you also don't overcomplicate the client creation. I think the only benefit of the approach I outlined is that it leaves the door open in case you need to override other settings in the client (e.g. maybe the That said, if we do end up needing that kind of functionality, we can always revisit the approach above or come up with a better way to override values. Ultimately a big concern for me is the user experience. I think both approaches are similar in terms of how you use the feature (i.e. add a |
I feel more comfortable with this solution now. Let's give it a try. I fixed a couple of issues where different clients were consuming messages from each other, leading to unsync start route and console from the wrong client. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Awesome! Thanks for getting those other issues resolved! |
What kind of change does this pull request introduce?
I'm trying to render two Preview components that point to different pages (e.g.
/about
and/careers
). This PR includes some rough changes and a Storybook story to better explain my intentions.The core change is to add a
startRoute
prop to the Preview component that overrides thestartRoute
option that's passed to the Provider. WhencreateClient
is called in theuseClient
hook, the override value is used instead.This idea could probably be extended for other things that may need an override, but for now, I explicitly exposed the
startRoute
functionality.This may be an unnecessary change.
Based on the Preview Component documentation:
If there's already a way to do this, could someone on the core Sandpack team point me in the correct direction?
Thanks!
What is the current behavior?
All Preview components start on the route defined by the
startRoute
option passed to the top-level Provider.What is the new behavior?
A new
startRoute
prop has been added to the Preview component that lets you override the top-level Provider option.What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.
I created a Storybook story
MultipleRoutePreviews
that uses React Router. It defines two routes and renders two Preview components that point to the different routes. When the story loads up, each Preview component starts on the correct route now.Checklist
This PR is still very rough. There may be better ways to accomplish what I'm trying to do and any proposed change would still require an update to the documentation.