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(theme): dark and light AppStore menu fix #198

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

mauris
Copy link
Contributor

@mauris mauris commented Jun 8, 2022

Fixes #172
Fixes #173
Fixes #169

Hi @rohit-gohri, thanks for this amazing plugin. I noticed the issues described in those issues filed and am proposing a fix.

The issue was because we are initiating 2 copies of AppStore: one for dark and one for light. The last copy of AppStore successfully binds various scroll events to the window and hence was the only one able to receive the event. When in light theme, this doesn't work. I understood from the code that the 2 copies of AppStore were meant for ServerStyles to generate the dark and light stylesheets correctly.

I propose to only initiate 1 copy of AppStore and depending on the theme, update the options accordingly.

Feel free to clarify on the changes proposed. If this is alright, I'd like this fix to be published as soon as possible.

@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2022

⚠️ No Changeset found

Latest commit: 30f4d88

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jun 8, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @rohit-gohri on Vercel.

@rohit-gohri first needs to authorize it.

@vercel
Copy link

vercel bot commented Jun 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
redocusaurus ✅ Ready (Inspect) Visit Preview Jun 13, 2022 at 7:14AM (UTC)

@mauris
Copy link
Contributor Author

mauris commented Jun 9, 2022

@oil-oil @srweeks @birkmose does the change fix the issue you've described? You can look at it from the preview: https://redocusaurus-git-fork-mauris-fix-store-menu-problem-rohit-gohri.vercel.app/

@srweeks
Copy link

srweeks commented Jun 9, 2022

it does, nice. Noticing that switching between themes no longer maintains the selection though

@birkmose
Copy link

@mauris doesn't seem to fix the menu expansion issue no :(
Example: https://redocusaurus-git-fork-mauris-fix-store-menu-problem-rohit-gohri.vercel.app/examples/using-swagger-json/#tag/user/operation/createUsersWithArrayInput
When opening this link the left-hand menu stays collapsed.

@mauris
Copy link
Contributor Author

mauris commented Jun 13, 2022

added more changes that should address issues raised by @birkmose and @srweeks about switching themes and "not expanding menu sometimes"

Thanks fellow collaborators for taking a look.

the key issue was that, in redoc, the AppStore instance is expected to be kind of a singleton. If you need to change the AppStore because some options were changed, you need to AppStore.dispose() the current instance, and reinitialize a new AppStore by running new AppStore(...).

What I've done in the last commit, was to simply store the current singleton instance outside of the hook, and make sure to clean up the current instance of AppStore before initializing a new one.

I've also added useEffect to watch theme change and when first loaded on browser to perform a AppStore.onDidMount() to ensure that the menu expands correctly.

@rohit-gohri
Copy link
Owner

What I've done in the last commit, was to simply store the current singleton instance outside of the hook, and make sure to clean up the current instance of AppStore before initializing a new one.

That's a good solution but that will break with multiple instance of Redoc are on a single page which is possible when using the MDX components to render schemas like here: https://redocusaurus.vercel.app/docs/guides/schema-imports

I'm not sure what the solution is, but this approach is definitely in the right direction and fixing some of the "hacks" I had added before. Maybe I should separate the hooks for the MDX embeds and full page docs?

@rohit-gohri rohit-gohri self-requested a review June 13, 2022 06:34
@mauris
Copy link
Contributor Author

mauris commented Jun 13, 2022

tested ApiSchema on my local build and it looked fine even with the singleton, likely because ApiSchema only uses values from the options and didn't really depend on other content such as the Redoc left side menu.

is it possible to update the preview? @rohit-gohri thanks!

@mauris
Copy link
Contributor Author

mauris commented Jun 13, 2022

Thanks @rohit-gohri, the preview deployment succeeded and I can see the issues described by @birkmose and @srweeks were resolved. ApiSchema also seem to be working without issue. Able to confirm? Thanks

@rohit-gohri rohit-gohri added bug Something isn't working integration labels Jun 14, 2022
@rohit-gohri
Copy link
Owner

These changes look good to me, I will be merging it to a temporary branch to verify the percy snapshot tests to catch any visual regressions. And then release if all goes well

@rohit-gohri rohit-gohri changed the base branch from main to develop June 14, 2022 17:37
@rohit-gohri rohit-gohri merged commit f3cca29 into rohit-gohri:develop Jun 14, 2022
This was referenced Jun 14, 2022
@rohit-gohri
Copy link
Owner

Released in v1.1.3! Thank you for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working integration
Projects
None yet
4 participants