-
Notifications
You must be signed in to change notification settings - Fork 1
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
397 as a dev I want to have a dev menu for demo purposes #399
base: main
Are you sure you want to change the base?
397 as a dev I want to have a dev menu for demo purposes #399
Conversation
add devstore add fill field with data add dev menu add dev menu when user demo is selected in login modal
move labelData.json to the root of the public folder make the button appear only on the good page
remove demomessage from layout.tsx Need to fix for labelDataValidator
3bca662
to
5326863
Compare
5326863
to
b72d8a8
Compare
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.
Looks like you need to run the code formatter for the files
@@ -10,86 +10,103 @@ import axios from "axios"; | |||
import Cookies from "js-cookie"; | |||
import { useRouter } from "next/navigation"; | |||
import { useEffect, useState } from "react"; | |||
import useDevStore from "@/stores/devStore"; | |||
|
|||
function LabelDataValidationPage() { |
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.
might as well update this to arrow function syntax
const LabelDataValidationPage = () =>
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.
done
src/app/page.tsx
Outdated
@@ -11,6 +12,7 @@ import { useTranslation } from "react-i18next"; | |||
function HomePage() { |
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.
use arrow function syntax
const HomePage = () =>
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.
done
src/components/DevMenu.tsx
Outdated
if ( | ||
key === "errors" && | ||
typeof obj[key] === "object" && | ||
obj[key] !== null | ||
) { |
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.
null check should probably go before the type check
also better to use != null
instead of !== null
because != null
will work with both null and undefined
if (
key === "errors" &&
obj[key] != null &&
typeof obj[key] === "object"
) {
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.
done
src/components/DevMenu.tsx
Outdated
} | ||
} else if (typeof obj[key] === "object" && obj[key] !== null) { | ||
const nestedErrors = extractErrorKeys(obj[key] as Record<string, unknown>, currentKey); |
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.
} else if (obj[key] != null && typeof obj[key] === "object") {
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.
done
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.
Overall we should avoid coupling the dev menu code with the real code to avoid breaking stuff. That would also ease developing both independently.
On that premise,
- instead of coupling the visibility of the dev menu with the login / sign up process, we could try tying it to the cookie
- instead of embedding the label data loading into the existing code, we could try creating dev mode twins for the relevant api routes
- there probably should be an option to change the backend url
}; | ||
fetchData(); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Let's try to avoid disabling lints as much as possible.
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.
Yeah I'm agree but those are needed to avoid adding other const to the use effect that cause error.
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.
It is for avoiding a warning.
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.
done
}); | ||
login(username, password).then((message) => { | ||
if (message === "" && username === "demoFertiscan"){ | ||
useDevStore.getState().setIsDemoUser(true); |
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.
Have you tested that this is working as expected?
setErrorMessage(message); | ||
}); | ||
login(username, password).then((message) => { | ||
if (message === "" && username === "demoFertiscan"){ |
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.
What about sign up? Because our sign up process logs in the user automatically.
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.
done
useDevStore.getState().setIsDemoUser(true); | ||
console.log("Dev mod enabled"); | ||
}else{ | ||
console.log(message); |
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.
Have you tested that when we log out and log a user other than demoFertiscan
in an environment other than development
during the same session, the dev menu doesn't show?
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.
done
}); | ||
setLabelData({ ...labelData }); | ||
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Let's try to avoid disabling lints as much as possible. There must be a workaround to achieve what you want.
setErrorMessage(message); | ||
}); | ||
login(username, password).then((message) => { | ||
if (message === "" && username === "demoFertiscan"){ |
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.
Instead of a checking a single user, we could check a prefix like demo_
. That would give us more options. Moreover, consider using an env var to avoid hardcoding.
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.
No the point of this is that the use can use it when showing the app during reunions. But yes the env can be a good idea to add.
…n when in dev env
DevMenu
Feature:
To active the devMenu you need to create a use call "demoFertiscan" and log with it.