Refactored Header/MobileNav.jsx with all Todos #3312
Conversation
| const project = useSelector((state) => state.project); | ||
| const user = useSelector((state) => state.user); | ||
|
|
||
| const { t } = useTranslation(); | ||
| // console.log('The title: ', title); |
There was a problem hiding this comment.
Ideally, we want to isolate changes in each pull request to tie as much to the original issue as possible. In most scenarios, we recommend not having pull requests build on top of another and create dependencies on each other.
There was a problem hiding this comment.
By update i meant i started from scratch but worked on the same problem, just with a different idea in mind.
i didn't used any code from previous pull request.
| const rootFile = useSelector( | ||
| (state) => state.files.filter((file) => file.name === 'root')[0] | ||
| ); | ||
| // DONE |
There was a problem hiding this comment.
If a comment is addressed, the original one can simply be deleted!
| @@ -244,7 +249,7 @@ const MobileNav = () => { | |||
| )} | |||
| </Title> | |||
| {/* check if the user is in login page */} | |||
| {pageName === 'login' || pageName === 'signup' ? ( | |||
| {pageName === 'LoginView.Login' || pageName === 'LoginView.SignUp' ? ( | |||
There was a problem hiding this comment.
Replacing hardcoded strings is a great move here!
| @@ -266,7 +271,7 @@ const MobileNav = () => { | |||
| </Link> | |||
| </div> | |||
| )} | |||
| {pageName === 'home' ? ( | |||
| {pageName === 'home' || pageName === project.name ? ( | |||
There was a problem hiding this comment.
This could probably look more lik {pageName === project.name ? ( to remove the hardcoded string!
| const isMobile = useIsMobile(); | ||
|
|
||
| // console.log('The updated mobile title is: ', mobileTitle); |
There was a problem hiding this comment.
We would ideally want to remove commented out logs!
|
I'm sorry, as mentioned in your other PR, we ideally want to accept pull requests that are tied to an issue. However, I appreciate the consideration for accessibility and attention to what the repository might be needing! I also added in a few comments regarding what potential changes might look like. I'm sorry again that we couldn't get this in, but please feel free to check out the issues within this repository! |
Refactor: Updated "MobileNav.jsx" file with all the todos
Changes:
Now component accepts the mobileTitle prop (handled default value and prop type)
An update to PR Refactor MobileNav for handling mobile title more efficiently #3310
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123