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: Import & Export from Github causes reloading the playground even before accept this step. #1908

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

ajotka
Copy link
Contributor

@ajotka ajotka commented Oct 16, 2024

Motivation for the change, related issues

Issue #1902

Implementation details

Import & Export from Github causes reloading the playground even before accept this step.
I know that it is because of adding new params in the URL.
So I kept functionality to fire those modal via URL, but I also added logic to open those modals without adding params.

Testing Instructions (or ideally a Blueprint)

  1. open http://127.0.0.1:5400/website-server/
  2. Go to playgrund settings
  3. Click 3 dots next to "Homepage" button
  4. Click export or import from GitHub
  5. Modal should appear without reloading

Also you can check, that opening on slug is still working (but with reloading on closing popup):
http://127.0.0.1:5400/website-server/?state=github-import

import { PlaygroundDispatch } from '../../lib/state/redux/store';
import { useDispatch } from 'react-redux';
import { useEffect, useState } from 'react';
import { setActiveModal } from '../../lib/state/redux/slice-ui';

const query = new URLSearchParams(window.location.search);
export const isGitHubExportModalOpen = signal(
query.get('state') === 'github-export'
Copy link
Collaborator

@adamziel adamziel Oct 16, 2024

Choose a reason for hiding this comment

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

It seems like we could get rid of this signal and initialize the activeModal redux state using the github-export query parameter. This would also:

  • Fix the linter warnings about useEffect relying on isGitHubExportModalOpen.value.
  • Make the state transitions read nicer, e.g. just dispatch(setActiveModal(null)); instead of toggleModal(false); closeModal(); dispatch(setActiveModal(null));.

For context – I introduced the signal way before we had the redux store and never got to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Reviewed
Development

Successfully merging this pull request may close these issues.

2 participants