Refactor MobileNav for handling mobile title more efficiently #3310
Conversation
There was a problem hiding this comment.
Thanks so much for giving this a shot! I edited it contained text meant for a different issue, I'm sorry about that!
Although this pull request addresses a TODO written within a file, we ideally want to accept pull requests that are associated to an issue to ensure that contributors can work on issues that have been confirmed by maintainers. I'm sorry that we couldn't get this in and that the process might've been a bit confusing, but I strongly encourage finding an issue to work within the repository vs. looking through the code!
| } | ||
|
|
||
| const title = useMemo(resolveTitle, [pageName, project.name]); | ||
| // function resolveTitle() { |
There was a problem hiding this comment.
Although we can't get this in, I wanted to leave a few notes for your work!
Ideally, we want to ensure that comments provide supportive information. Since this function is being removed, ideally lines 212 and 213 - 230 could simply be deleted. Since this issue is attempting to address the TODO, line 212 could be removed as well.
| }; | ||
|
|
||
| // Add a console warning in development | ||
| if (process.env.NODE_ENV === 'development') { |
There was a problem hiding this comment.
The changes you applied to the useWhatPage.js hook should be visible within development, so I don't think this warning might be needed here!
There was a problem hiding this comment.
Thanks for taking the time to review my code! I appreciate your feedback and learned something from it. I'll make the necessary improvements. Thanks again!
Refactior: TODO: remove the switch and use a props like mobileTitle
Changes:
useWhatPagehook to maintain clean and modular code while ensuring the app's functionality remains intact.MobileTitleas a prop, I shifted the logic to theuseWhatPagehook. This approach provides a cleaner and more optimized solution by deriving the title directly from the function.I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123