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.

Sign up for GitHub

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

Refactor MobileNav for handling mobile title more efficiently #3310

Closed
wants to merge 2 commits into from
Closed

Refactor MobileNav for handling mobile title more efficiently #3310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7255645
Refactored useWhatPage hook to provide mobileTitle
Rajnish2105 Dec 25, 2024
e5252b2
Removed accidental test insertion on user/pages for AccountView
Rajnish2105 Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions client/modules/IDE/components/Header/MobileNav.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useContext, useMemo, useState } from 'react';
import React, { useContext, useState } from 'react';
import styled from 'styled-components';
import { useDispatch, useSelector } from 'react-redux';
import { useTranslation } from 'react-i18next';
Expand Down Expand Up @@ -204,30 +204,30 @@ const MobileNav = () => {
const project = useSelector((state) => state.project);
const user = useSelector((state) => state.user);

const { t } = useTranslation();
// const { t } = useTranslation();

const editorLink = useSelector(selectSketchPath);
const pageName = useWhatPage();
const { pageName, title } = useWhatPage();

// TODO: remove the switch and use a props like mobileTitle <Nav layout=“dashboard” mobileTitle={t(‘Login’)} />
function resolveTitle() {
switch (pageName) {
case 'login':
return t('LoginView.Login');
case 'signup':
return t('LoginView.SignUp');
case 'account':
return t('AccountView.Settings');
case 'examples':
return t('Nav.File.Examples');
case 'myStuff':
return 'My Stuff';
default:
return project.name;
}
}

const title = useMemo(resolveTitle, [pageName, project.name]);
// function resolveTitle() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

// switch (pageName) {
// case 'login':
// return t('LoginView.Login');
// case 'signup':
// return t('LoginView.SignUp');
// case 'account':
// return t('AccountView.Settings');
// case 'examples':
// return t('Nav.File.Examples');
// case 'myStuff':
// return 'My Stuff';
// default:
// return project.name;
// }
// }

// const title = useMemo(resolveTitle, [pageName, project.name]);

const Logo = AsteriskIcon;
return (
Expand Down
46 changes: 43 additions & 3 deletions client/modules/IDE/hooks/useWhatPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,33 @@
import { useMemo } from 'react';
import { useSelector } from 'react-redux';
import { useLocation } from 'react-router-dom';
import { useTranslation } from 'react-i18next';

const getPageTitle = (pageName, t) => {
switch (pageName) {
case 'login':
return t('LoginView.Login');
case 'signup':
return t('LoginView.SignUp');
case 'account':
return t('AccountView.Settings');
case 'examples':
return t('Nav.File.Examples');
case 'myStuff':
return 'My Stuff';
case 'home':
default:
return null; // Will use project.name as fallback in Nav
}
};

/**
*
* @returns {"home" | "myStuff" | "login" | "signup" | "account" | "examples"}
* @returns {{ page: "home" | "myStuff" | "login" | "signup" | "account" | "examples", title: string }}
*/
const useWhatPage = () => {
const username = useSelector((state) => state.user.username);
const { pathname } = useLocation();
const { t } = useTranslation();

const pageName = useMemo(() => {
const myStuffPattern = new RegExp(
Expand All @@ -24,7 +43,28 @@ const useWhatPage = () => {
return 'home';
}, [pathname, username]);

return pageName;
const title = useMemo(() => getPageTitle(pageName, t), [pageName, t]);

// For backwards compatibility
const returnValue = {
pageName,
title
};

// Add a console warning in development
if (process.env.NODE_ENV === 'development') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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!

// Make the object look like a string for old code
Object.defineProperty(returnValue, 'toString', {
value: () => {
console.warn(
'Deprecated: useWhatPage() now returns an object. Please update your code to use { page, title }.'
);
return pageName;
}
});
}

return returnValue;
};

export default useWhatPage;