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

Feature/12 #38

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Feature/12 #38

wants to merge 9 commits into from

Conversation

louisan42
Copy link
Contributor

What

Adds sign in page and account setup pages

Why

This adds a core feature to the application

How

By creating a sign in page using google sign in and an account setup page which will serve as a global config for registered users.

Covers

#12

Checklist

  • Are the issues being addressed linked to this PR?
  • Do all commit messages start with the issue number?
  • Are all code changes sufficiently tested?
  • Are there screenshots for UI changes?

Screenshots

Desktop view

sign_in

accountSetup

Mobile view

image

Copy link
Collaborator

@ryanolee ryanolee left a comment

Choose a reason for hiding this comment

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

Great job a few functional comments (Mainly surrounding the form validation)
But otherwise looking really good Great job @louisan42 ,
Let me know if you need any help from me fixing any of these comments / want me to apply any of the fixes myself 👍

src/App.jsx Outdated Show resolved Hide resolved
src/components/button/CustomButton.jsx Outdated Show resolved Hide resolved
src/pages/accountSetup/AccountSetupPage.jsx Outdated Show resolved Hide resolved
src/components/container/CustomContainer.jsx Outdated Show resolved Hide resolved
src/components/customSelect/CustomSelect.jsx Outdated Show resolved Hide resolved
helperText={
<StyledFormHelperText id="main-text-font-helper-text">
{formik.touched.mainTextFont &&
formik.errors.mainTextFont}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The errors for this do not seem to work for some reason Select the main text font. I wonder if formik.touched.mainTextFont has not been set given if you try and submit the form with these empty no error ever appears. The form is just submittable with feels a little odd

await updateDoc(docRef,{ userPreferences: values });
navigate("/");
} catch (e) {
console.error(e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth dropping a https://mui.com/material-ui/react-alert/ just reporting that an error occurred on error

systemName: "",
twoFactAuth: false,
},
validationSchema: accountSetupSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some reson schema validation errors do not seem to be appearing in the formik error text. Let me know if you want a quick chat / for us to pair up and look into this

<Page showSideBar={false}>
<CustomContainer>
<Box sx={{ padding: "50px 100px" }}>
<Box sx={{ textAlign: "center", paddingBottom: "3rem" }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth dropping the font size on smaller view ports in order to allow for smaller mobile clients to see the page.

});

//Define options to pass to custom select component
const headerOptions = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thease can probably be defined outside the the component so we do not have to redefine them on rerender

louisan42 and others added 2 commits February 4, 2023 14:52
feature 12: init code clean up after review

Co-authored-by: ryanolee <landgraabiv@gmail.com>
feature 12: clear spacing and commented lines

Co-authored-by: ryanolee <landgraabiv@gmail.com>
@ryanolee
Copy link
Collaborator

@louisan42 I might extract the "Custom" components into another PR and rebase this one onto that one so that we can begin using the form components made here sooner 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants