Improve header 10 #598
Improve header 10 #598
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
|
| }, [isOrdersPanelOpen, isMobileMenuOpen, isUpToLarge]) | ||
|
|
||
| const getMainMenu = useMemo( | ||
| () => | ||
| MAIN_MENU.map(({ title, url, externalURL, items }: MAIN_MENU_TYPE, index) => |
There was a problem hiding this comment.
I'm sure @anxolin will love this part 😅
Anyway, would you be able to simplify this a bit?
Maybe use intermediate components or callbacks to reduce a bit the conditional chaining
There was a problem hiding this comment.
@alfetopito If we can simplify it further, that'd be great. Would any of you mind checking PR #596 (the current last one) and suggest how to further simplify? I know I did some changes up to the last PR.
There was a problem hiding this comment.
Yes, this is way too complex...
we can break it up a bit into smaller pieces. A map inside another map and with all these ternary operators are big red 🚩
There was a problem hiding this comment.
I'll try to break it down into smaller functions 👍🏼
* Move URLS to an ENUM * Fix enum issue. * Styling and route fixes. * Styling. * Styling. * Header/Menu improvement (waterfall PR FINAL) (#596) * Styling. * Styling footer and bridge banner. * Improve header 13 (#610) * Fix network selector position. * Fix exports/imports for network selector. * Only open ordersPanel if account is true.
| @@ -48,7 +49,7 @@ function RequestCancellationModal(props: RequestCancellationModalProps): JSX.Ele | |||
| <p> | |||
| This means that a solver might already have included the order in a solution even if this cancellation | |||
| is successful. Read more in the{' '} | |||
| <a target="_blank" href="/#/faq/trading#can-i-cancel-an-order"> | |||
| <a target="_blank" rel="noreferrer" href={`${Routes.FAQ_TRADING}#can-i-cancel-an-order`}> | |||
There was a problem hiding this comment.
I think we usually create the links using or <ExternalLink
is not part of this PR, but just curious if we know why this is like this?
There was a problem hiding this comment.
Yeah would need to change for sure. In this case I had simply changed the URL but should use the component 👍🏼
There was a problem hiding this comment.
Just tested this one out when the cancellation modal is open:
- using the component it opens the page below, which is not great.
- using the component I tried not to specify the root domain, but the URL gets malformed:
http://localhost:3000/faq/trading#/can-i-cancel-an-order?chain=rinkeby
thinking to leave as is for now in this iteration
| }, [isOrdersPanelOpen, isMobileMenuOpen, isUpToLarge]) | ||
|
|
||
| const getMainMenu = useMemo( | ||
| () => | ||
| MAIN_MENU.map(({ title, url, externalURL, items }: MAIN_MENU_TYPE, index) => |
There was a problem hiding this comment.
Yes, this is way too complex...
we can break it up a bit into smaller pieces. A map inside another map and with all these ternary operators are big red 🚩
| import IMAGE_SLICER from 'assets/cow-swap/ninja-cow.png' | ||
| import IMAGE_GAME from 'assets/cow-swap/game.gif' | ||
|
|
||
| export interface MAIN_MENU_TYPE { |
There was a problem hiding this comment.
I wonder if these constants should be instead near the menu component, instead of in the general constants
There was a problem hiding this comment.
In this case the icons are specific to the menu tree structure, which comes from the mainMenu types. So my thinking is it should be closely tied to the menu structure. WDYT?
| @@ -0,0 +1,24 @@ | |||
| // ENUM with routes | |||
| export enum Routes { | |||
There was a problem hiding this comment.
@fairlighteth this I like 👍
we should make sure the all our links start to use this enums. Maybe we can even create a Link which only accepts this enum as the path parameter
Summary
mainMenu.ts. A single file which can be used to define the main menu as a while.const getMainMenustructure with conditional logic supporting the mainMenu.ts constant data structure.toggleBodyClass.tsto toggle thedocument.bodyclass, with the class provided as an argument. Used to toggle the 'noScroll` class whenever an overlay (like the main menu on mobile) is active.