Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Header #5

Merged
merged 9 commits into from
Mar 25, 2021
Merged

Header #5

merged 9 commits into from
Mar 25, 2021

Conversation

lbratkovskaya
Copy link
Owner

No description provided.

AlexeyTeterin
AlexeyTeterin requested changes Mar 22, 2021
View reviewed changes
Copy link
Collaborator

@AlexeyTeterin AlexeyTeterin left a comment

Choose a reason for hiding this comment

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

Предлагаю вместо classes.ts обозвать их styles.ts, все-таки classes - слишком многозначное слово )

AlexeyTeterin
AlexeyTeterin reviewed Mar 23, 2021
View reviewed changes
src/components/Header/MobileNavMenu.tsx Outdated
setTimeout(() => setSubMenuOpenId('none'), 0);
};

const { anchor, isOpen, items, onMenuClose } = props;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Если я не ошибаюсь, пропсы лучше в самое начало поднять

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

AlexeyTeterin
AlexeyTeterin reviewed Mar 23, 2021
View reviewed changes
src/components/Header/navMenuData.ts Outdated
},
];

export const teamsItems = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Они же нам больше не нужны?

Copy link
Owner Author

Choose a reason for hiding this comment

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

убраны

Tonia-SE
Tonia-SE reviewed Mar 23, 2021
View reviewed changes
src/components/Header/navMenuData.ts Outdated
const teamsId = 'teams';
export const mobileMenuId = 'mobileMenu';

export const gamesItems: ISubMenuItem[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Здесь и далее, выражение типа ISubMenuItem[] можно заменить, например, Array и прописать интерфейс IMenuItem

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

src/components/Header/NavigationMenu.tsx Outdated
{items && renderItems()}
</Menu>
);
};

export const NavigationMenu: React.FC = () => {
const [subMenuOpenId, setSubMenuOpenId] = useState<string>('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

useState(''), зачем дополнительно типизовать useState? Ты же ему и так string скормила?

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

AlexeyTeterin
AlexeyTeterin requested changes Mar 24, 2021
View reviewed changes
src/components/Header/types.ts
export type ISubMenuItem = {
label: string;
withLink: boolean;
linkAddress?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

linkAdress (лишняя 'd')

Copy link
Owner Author

Choose a reason for hiding this comment

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

Но нет)
https://www.multitran.com/m.exe?l1=1&l2=2&CL=1&s=address

@lbratkovskaya lbratkovskaya merged commit 3a6b57e into develop Mar 25, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Gurnick013 Gurnick013 Gurnick013 approved these changes

@blackymax blackymax blackymax approved these changes

@alexlatypov94 alexlatypov94 alexlatypov94 approved these changes

@Tonia-SE Tonia-SE Tonia-SE approved these changes

@AlexeyTeterin AlexeyTeterin AlexeyTeterin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lbratkovskaya @AlexeyTeterin @Tonia-SE @alexlatypov94 @blackymax @Gurnick013