-
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
Granthamblin0/tces 9 urd navbar #23
Conversation
TCES-9 URD-Navbar
Screenshot 2023-10-14 at 3.42.32 PM.png Screenshot 2023-10-14 at 3.44.21 PM.png Users need to navigate our app!
Completion Requirements:
|
1c8ac44
to
8cf2f20
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.
Everything is coming along great. Just a few comments, but nothing super major.
When you get the change, be sure to have Jake review it. This can be done by adding screenshots of all its functionality to the PR description and sharing the link with him.
When it is approved, you can update the label to Designer Approved.
Thank you for the hard work so far!
package.json |
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.
We do not want to be adding anything to the ignore as the jsx files should always be checked for style and linting
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.
All the icons could be removed and you can import mui-icons. This will keep everything more streamlined and unclutter our media files. I have an example in the linter branch on the NavbarProfile.jsx
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.
Great job Grant!
I just left a few minor comments. Also as a general comment, while some of the links don't have pages to direct to, they should still be clickable and lead to some dummy route for the time being ("#" for example). Also when a user hovers over these links there should be some indication that they are clickable (for example cursor changing to pointer, etc.)
|
||
function Dropdown({ isAdmin }) { | ||
return ( | ||
<div className="dropdown-container"> |
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.
set z-index to some high number so that despite any other components being rendered on the page, the dropdown can be seen.
logout: <LogoutIcon color="action" />, | ||
admin: <AdminIcon color="action" />, | ||
}; | ||
|
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 would also recommend retrieving label from a dictionary that matches keyword to label as you have done with icons.
<div className="nav-container"> | ||
<div className="left-content"> | ||
<div className="image"> | ||
<img src={logo} alt="logo" width="46" height="50" /> |
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.
replace {logo} with "./img/tcesLogo.svg". svgs scale much better and are generally favourable over their png counterparts. Also please delete the tces-logo.png file from public/img
import "./Dropdown.css"; | ||
import DropdownItem from "./DropdownItem"; | ||
|
||
function Dropdown({ isAdmin }) { |
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.
Whenever a user clicks off of the dropdown it should close it
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.
Sorry I'm struggling with implementing this
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.
Daniel has all the main stuff covered in his comments so be sure to check those out.
Just reiterating, use MUI components and icons wherever possible to minimize the overhead for yourself. NavbarProfile.jsx
can be used as a guide for you to follow with the other components.
Also, definitely take a look at the Dashboard stuff if you need some more examples. Kevin did a really great job of building out the components with MUI and styled-components
Message us if you have any questions
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 good, just finish up the previous comments and it should be good to go.
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.
Great work Grant!
Everything is coming together really nicely! I just left a few small comments -- after resolving these I think we'll be good to merge your pr in.
client/src/App.jsx
Outdated
@@ -1,15 +1,15 @@ | |||
import "./App.css"; |
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.
revert changes in this file
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.
You could even just add the above the dashboard component
<Navbar />
<Dashboard />
{isAdmin && <DropdownItem keyword="admin" />} | ||
<DropdownItem keyword="settings" /> | ||
<DropdownItem keyword="logout" /> |
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.
Ideally these are clickable rather than just the arrow contained within them
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 looking good. Just added to one of Daniel's new comments but then we should be good to go.
This all came together really nicely. Should be just quick changes!
Thanks Grant!
client/src/App.jsx
Outdated
@@ -1,15 +1,15 @@ | |||
import "./App.css"; |
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.
You could even just add the above the dashboard component
<Navbar />
<Dashboard />
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.
Lit
* Completing TCES-38 - added name * Completing TCES-9 Finished Navbar * Completing TCES-9 Fixed a few things * Completing TCES-9 - sorry forgot to use linter * TCES-9 Fixed most problems * TCES-9 final final * TCES-9 Reverted app.jsx changes * Updated the profile dropdown * TCES-9 formatting * TCES-9 more formatting --------- Co-authored-by: Grant Hamblin <[email protected]> Co-authored-by: Daniel Dervishi <[email protected]>
* Completing TCES-38 - added name * Completing TCES-9 Finished Navbar * Completing TCES-9 Fixed a few things * Completing TCES-9 - sorry forgot to use linter * TCES-9 Fixed most problems * TCES-9 final final * TCES-9 Reverted app.jsx changes * Updated the profile dropdown * TCES-9 formatting * TCES-9 more formatting --------- Co-authored-by: Grant Hamblin <[email protected]> Co-authored-by: Daniel Dervishi <[email protected]>
* TCES-28 Added job lead component * TCES-28 Modified button dashes * TCES-28 Fixed date inputs * TCES-12 User Login, Logout, and Basic Authorization (#26) * TCES-37 Add Ron Varshavsky to readme * TCES-12 feat(auth): add skeleton endpoints for auth api * TCES-12 fix(auth): send response on post request * TCES-12 feat(auth) add PassportJS packages * TCES-12 config(auth) add LocalStrategy config to auth.js * TCES-12 feat(auth) add passport middleware and fix SQL bug * add constants * TCES-12 config(user, frontend) update config variables for frontend and auth * TCES-12 config(index) update express to use json body requests * TCES-12 feat(create-user) implements create_user endpoint * TCES-12 refactor(create_user) fix indentation * TCES-12 feat(login) implements user login * TCES-12 feat(session) implements user session * TCES-12 feat(logout) implements user logout * TCES-12 refactor(auth) replace cookie authentication with server-side sqlite auth storage * TCES-12 feat(auth) add admin feature * TCES-12 feat(auth) add admin control for creating a user * TCES-12 fix(auth) fix error status response * TCES-12 refactor(auth) rewrite functions to use arrow functions * TCES-12 refactor(auth) restructure config file for passport * TCES-12 fix(auth) remove automatic logging in when user created * TCES-12 refactor(auth) remove callback function for hashing * TCES-12 hotfix(auth) fix hashing function * TCES-12 refactor(auth) implement sequelize model for auth * TCES-12 refactor(auth) implement sequelize for user * TCES-12 refactor(auth) implement sequelize in separate file * TCES-12 refactor(auth) refactor localstrategy imports * TCES-12 fix(auth) implements user login feature * TCES-12 refactor(auth) update .env.example to remove credentials * TCES-12 refactor(auth) update .env.example, index, and update routing * TCES-12 fix(auth) update model to match sql * TCES-12 refactor(auth) refactor all routes to use controller * TCES-12 refactor(auth) refactor all controllers to be separate files * TCES-12 refactor(auth) refactor after formatter and linter * TCES-12 test(auth) add test cases for createUser * TCES-12 refactor(auth_test) refactor test cases for auth to match formatter and linter * TCES-12 hotfix(auth_test) fix sequelize connection failure to pass tests * TCES-12 hotfix(auth_test) fix sequelize connection failure to pass tests * TCES-12 hotfix(auth_test) fix sequelize connection failure to pass tests * TCES-12 refactor(auth_test) add env variables to github runner * TCES-12 refactor(auth_test) add env variables to github runner * TCES-12 refactor(auth_test) add env variables to github runner * TCES-12 refactor(auth) update runner env and make edits for code review * TCES-12 refactor(auth) add authentication middleware instead of authenticating in controller * TCES-12 refactor(auth) refactor to json * TCES-12 refactor(auth) refactor middleware to make testing easier * TCES-12 test(auth) add testing for middleware * TCES-12 refactor(auth) refactor for autoformatter * TCES-12 refactor(auth) refactor to match JSend standards * Completing TCES-11 URD-Admin Dashboard with User Table (#25) * Completing TCES-11 Implemented the datagrid * Completing TCES-11 Fully implemented the page * Completing TCES-11 Turned off default filtering/sorting * Completing TCES-11 FIxed code * Completing TCES-11 Used styled components * TCES-28 Added job lead component * TCES-28 Updated employment types * TCES-27 Created components for each page * TCES-27 Fixed input reset upon refresh * TCES-27 Added DISCARD button functionality * TCES-27 Added routing and button functionality * TCES-27 Fixed routing * TCES-27 Switched from routing to useState * TCES-27 Fixed page indicator * TCES-27 idk why my other component was here haha * TCES-27 removed console logs * TCES-27 Temporarily disable eslint import/no-cycle * Completing TCES-11 URD-Admin Dashboard with User Table (#25) * Completing TCES-11 Implemented the datagrid * Completing TCES-11 Fully implemented the page * Completing TCES-11 Turned off default filtering/sorting * Completing TCES-11 FIxed code * Completing TCES-11 Used styled components * Jordanjanakievski/add code coverage for testing (#28) * Add coverage library for testing * Add additional tests for isAdmin and isLoggedIn * Add mocking for function calls * Remove the extra tests * Granthamblin0/tces 9 urd navbar (#23) * Completing TCES-38 - added name * Completing TCES-9 Finished Navbar * Completing TCES-9 Fixed a few things * Completing TCES-9 - sorry forgot to use linter * TCES-9 Fixed most problems * TCES-9 final final * TCES-9 Reverted app.jsx changes * Updated the profile dropdown * TCES-9 formatting * TCES-9 more formatting --------- Co-authored-by: Grant Hamblin <[email protected]> Co-authored-by: Daniel Dervishi <[email protected]> * TCES-28 Added job lead component * Completing TCES-11 URD-Admin Dashboard with User Table (#25) * Completing TCES-11 Implemented the datagrid * Completing TCES-11 Fully implemented the page * Completing TCES-11 Turned off default filtering/sorting * Completing TCES-11 FIxed code * Completing TCES-11 Used styled components * TCES-28 Added job lead component * TCES-27 Added routing and button functionality * TCES-27 Fixed page indicator * TCES-27 Fixed Spacing * TCES-27 Removed extra imports * TCES-27 Fixed merge from main * TCES-27 Fixed npm install * TCES-27 Added cursor pointer on button * TCES-27 Changed parameter name * TCES-27 Added pagination * TCES-27 Fixed linter issue * TCES-27 Added input changes * TCES-27 Switched to session storage * TCES-27 Fixed style issue * TCES-27 Added parent component and page navigation * TCES-27 Changed date picker * TCES-27 Refactored localStorage in company-info * TCES-27 Refactored localStorage employer-contact * TCES-27 Refactored localStorage employer-contact * TCES-27 added imask for phone number inputs * TCES-27 Re-ran npm install * TCES-27 Added add employer page * TCES-27 Fixed empty select field * TCES-27 Fixed console errors * TCES-27 Fixed console error * TCES-27 Modified input field ids * TCES-27 Fixed select input labels * TCES-27 changed input to e * TCES-27 Set number inputs to type number * TCES-27 Combined job lead pr into this one * TCES-27 Capitalized New * TCES-27 Changes add job lead page name * TCES-27 Laid out general structure for refactor * TCES-27 Modified components for refactor * TCES-27 Modified headers * TCES-27 Commented out unused page --------- Co-authored-by: Ron Varshavsky <[email protected]> Co-authored-by: Selin Naz Taşman <[email protected]> Co-authored-by: Jordan Janakievski <[email protected]> Co-authored-by: Grant <[email protected]> Co-authored-by: Grant Hamblin <[email protected]> Co-authored-by: Daniel Dervishi <[email protected]>
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Issue #
Type of change
Please delete options that are not relevant.
Checklist
Please delete options that are not applicable.