-
Notifications
You must be signed in to change notification settings - Fork 34
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
will persist snippet data across the login session #400
base: main
Are you sure you want to change the base?
will persist snippet data across the login session #400
Conversation
src/hooks/use-global-store.ts
Outdated
interface snippetInfo { | ||
circuitJson?: AnyCircuitElement[] | null | ||
code: string | ||
snippetType: "board" | "package" | "model" | "footprint" | string | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet type shouldn't be undefined
or string
, when we are using the const names. It defeats the point of it
src/hooks/use-run-tsx/index.tsx
Outdated
@@ -26,7 +26,7 @@ export const useRunTsx = ({ | |||
}: { | |||
code?: string | |||
userImports?: Record<string, object> | |||
type?: "board" | "footprint" | "package" | "model" | |||
type?: "board" | "footprint" | "package" | "model" | string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, we shouldn't be using string
@Niharika0104 Can you write a playwright test for this? And it would be better if you can split into multiple files |
@imrishabh18 Okay sure.So we should split tests across multiple files ? |
Yeah, Playwright tests took too much time if one file is having too many steps because of the wait time. Rather than that a better approach could be that small tests file with less steps, and that will be executed in parallel |
94ee1ea
to
7d1cbd7
Compare
@imrishabh18 i'm unable to test these changes locally coz locally we are using fakeApi and not the redirecting the user to the github oauth page |
@Niharika0104 Yeah, correct. I think we can merge this without test. What do you think @seveibar? On the UI, you have converted the |
@imrishabh18 yeah when we click on login to save it will redirect to GitHub oauth page and on entering their credentials it will redirect them to editor page and the code editor and manual edits json file will be populated with the info we saved in guest snippet variable present in the local storage.May be I should change its ui to look more like button rather than a simple tag |
|
||
useEffect(() => { | ||
if (snippet?.code) { | ||
setCode(snippet.code) | ||
setLastRunCode(snippet.code) | ||
} else if (!isLoggedIn && guestSnippet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this breaks the loading of templates?
@Niharika0104 I think this should be still testable in playwright if you create a fake-api login route, the url is something like:
Overall i like the implementation, except i think most users will click "login" in the header rather than the badge. It's a bit of work but if you're down i would recommend pausing this PR, then implementing a PR that just allows logging in inside playwright tests (issue: #409) Overall it's a good idea to store the guest session inside the global store. But I'm cautious about the loading process- we might want to have a specialized hook or something that runs the appropriate state machine because the |
@Niharika0104 i was looking at the logic in a bit more detail, it seems like the playwright test should be able to login due to this login logic: |
Yeah the tests are fine now but when we click on login /sign up on the header it takes us to the home page(/) but when we click on the login to save button it will take us to GitHub oauth and then to the editor page. |
Yea i cant enable auth in preview We should use the same code/logic for both logins, there's a sign in hook that should handle it iirc |
Yeah Im aware of that hook let me try again |
Fixes - #399