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

TCES-41 urd create and update user information to be updated #21

Merged

Conversation

tsids
Copy link
Contributor

@tsids tsids commented Oct 30, 2023

Created the edit user and create user pages.

Completed TCES-41.

Design approved by Jake.

image
image

@linear
Copy link

linear bot commented Oct 30, 2023

TCES-41 URD-Create and Update User Information - TO BE UPDATED

You will be responsible for the frontend flow of creating a new user and updating an existing user's information

Completion Requirements:

  • Designer Approval
  • Following information above

Copy link
Collaborator

@jordanjanakievski jordanjanakievski left a comment

Choose a reason for hiding this comment

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

Just left a few comments. Some stuff is on our end by not creating a guide for the file structure that we wanted. Otherwise, it is looking good!

</a>
</header>
</div>
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an unnecessary tag

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't render two components without a fragment, so I guess for now it may be necessary.

Typography,
} from "@mui/material";

const styles = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you are going to use CSS styling, it would be best to use dedicated css files for these

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively you could look into styled components. https://styled-components.com/. You could take a look at either Kevin or Emily's prs for a reference.

Typography,
} from "@mui/material";

const styles = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same styling comment from Create

@@ -1,27 +1,12 @@
import logo from "./logo.svg";
import "./App.css";
import Create from "./components/Create";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be best to have these existing in a pages directory like how was brought up in stand-up so when it comes to routing we would route to only existing pages, not components. Since these create pages will exist on a route.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference, please take a look at Kevin's PR.

marginRight: "auto",
},
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a function for the Button onClick and a fake API call so when the backend gets implemented, it will be a quick edit and test to get it hooked up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also you should probably maintain the states of the different fields in this component so that we can track what the user has inputted and attach that to the API call. This also applies in edit.jsx

marginRight: "auto",
},
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same API comment from Create

@jordanjanakievski jordanjanakievski added Frontend Any Frontend Changes and removed Frontend Any Frontend Changes labels Nov 3, 2023
@jordanjanakievski
Copy link
Collaborator

jordanjanakievski commented Nov 3, 2023

Also, be sure to get approval from Jake. This is a requirement for frontend PRs. Once that happens, you can update the PR description and add a label for Design Approved

@tsids
Copy link
Contributor Author

tsids commented Nov 3, 2023

Is there a naming convention for the external css file? Also, will each component/page have its own css file, or is there a master css file that we are supposed to use?

@jordanjanakievski
Copy link
Collaborator

Is there a naming convention for the external css file? Also, will each component/page have its own css file, or is there a master css file that we are supposed to use?

Each page would get its own CSS file

Copy link
Collaborator

@DanielDervishi DanielDervishi left a comment

Choose a reason for hiding this comment

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

Great work Taha! The components look really great. There are some minor things that need to be changed, but once those are addressed, the pr should be good to merge in!

Typography,
} from "@mui/material";

const styles = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively you could look into styled components. https://styled-components.com/. You could take a look at either Kevin or Emily's prs for a reference.

@@ -1,27 +1,12 @@
import logo from "./logo.svg";
import "./App.css";
import Create from "./components/Create";
Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference, please take a look at Kevin's PR.

</a>
</header>
</div>
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't render two components without a fragment, so I guess for now it may be necessary.

marginRight: "auto",
},
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also you should probably maintain the states of the different fields in this component so that we can track what the user has inputted and attach that to the API call. This also applies in edit.jsx

@tsids tsids force-pushed the taha/tces-41-urd-create-and-update-user-information-to-be-updated branch from f293ff6 to e340b7c Compare November 6, 2023 15:28
@tsids tsids requested a review from kevinle623 November 6, 2023 16:20
Copy link
Contributor

@kevinle623 kevinle623 left a comment

Choose a reason for hiding this comment

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

Nice! Looks really good.

@tsids tsids added Frontend Any Frontend Changes Design Approved 🎨 labels Nov 7, 2023
Copy link
Collaborator

@DanielDervishi DanielDervishi left a comment

Choose a reason for hiding this comment

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

Thanks for the changes Taha, they look great! The only things that should be changed are the names of the create and edit components, as well as the folder they are in. Something like create-user-component and edit-user-component would be better.

@tsids tsids force-pushed the taha/tces-41-urd-create-and-update-user-information-to-be-updated branch from 3ff2daf to 5024726 Compare November 7, 2023 16:49
@tsids
Copy link
Contributor Author

tsids commented Nov 7, 2023

I updated the folders, but do you want to update the file names too?
It won't stay consistent with the dashboard and login components.

@DanielDervishi
Copy link
Collaborator

I updated the folders, but do you want to update the file names too? It won't stay consistent with the dashboard and login components.

Yes that would be great. Also, could you update the password fields in each of the components to hide the characters as the user types them (see example below).
Screenshot 2023-11-07 at 11 56 07 AM

Copy link
Collaborator

@DanielDervishi DanielDervishi left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I just left a single comment. Otherwise the pr should be good to go!

type="submit"
variant="contained"
size="large"
onClick={handleSave}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, despite the form validation onClick is called on the button. Instead of assigning the button's onClick method to handleClick, instead assign the encompassing form's onSave method to it. This should be done in the create component as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find anything about an onSave method, but I added it to onSubmit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, thats what I meant to say!

Copy link
Collaborator

@DanielDervishi DanielDervishi left a comment

Choose a reason for hiding this comment

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

Thanks for the great work Taha!

Copy link
Collaborator

@jordanjanakievski jordanjanakievski left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for addressing mine and Daniel's comments!

@tsids
Copy link
Contributor Author

tsids commented Nov 8, 2023

Should I squash and merge?

@DanielDervishi
Copy link
Collaborator

Should I squash and merge?

Yep!

@tsids tsids merged commit a949a6d into main Nov 8, 2023
2 checks passed
@tsids tsids deleted the taha/tces-41-urd-create-and-update-user-information-to-be-updated branch November 8, 2023 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants