mobile global nav #447
mobile global nav #447
Conversation
app/components/Button/index.tsx
Outdated
| const classes = ['button', className, tooltip && 'tooltip'].filter((i) => i).join(' ') | ||
| if (typeof action === 'string') { | ||
| return ( | ||
| <Link to={action} className={classes} data-tooltip={tooltip}> | ||
| {leftIcon && leftIcon} |
There was a problem hiding this comment.
I don't like how this is getting bloated. Why not just pass the icon in as a child, rather than having special parameters? So e.g.:
<Button>
<SomeIcon />
bla bla
</Button>
rather than
<Button leftIcon={<SomeIcon/>}>bla bla</Button>?
There was a problem hiding this comment.
that's how it was done..I forgot to remove that part of the code.
| <XLarge className={'menu-button'} onClick={toggleMenu} /> | ||
| </nav> | ||
|
|
||
| <div className={'articles-mobile'}> |
There was a problem hiding this comment.
this should reuse the ArticlesDropdown component. A bit of extra css there should handle transforming it from a grid into a list
| <header className="top-header"> | ||
| {!showArticles && ( | ||
| <> | ||
| <nav className="top-nav"> |
There was a problem hiding this comment.
how much of this could be handled with css in the normal Nav component?
There was a problem hiding this comment.
I'm not sure I follow, can you please explain what you mean?
There was a problem hiding this comment.
This is basically the same as the current Nav, with a bit of things moved around. So you could have a mobile-only class that you add to the show menu button and a desktop-only one on the searchbar, which (with a couple of extra css settings) will allow you to have all of these in the same component, rather than duplicating them. The menu in theory could also be done with css, but that might be tricky. Still - there would then only be a few new items, which will make things simpler
There was a problem hiding this comment.
I only just noticed that I didn't send this comment...
There was a problem hiding this comment.
do you still want this done...?
There was a problem hiding this comment.
preferably. But this certainly still needs to be connected to the actual page
app/components/Nav/Mobile/index.tsx
Outdated
| {item.title} | ||
| </Link> | ||
| ))} | ||
| <div className="default-bold margin-bottom top-margin">Browse by category</div> |
There was a problem hiding this comment.
what are the classes margin-bottom and top-margin? For bottom padding we want to use the class "padding-bottom-[number]". Maybe I will create padding-top classes and even margin classes too...
| } | ||
| .top-nav { | ||
| justify-content: space-between; | ||
| padding: var(--spacing-20) var(--spacing-24); |
There was a problem hiding this comment.
The nav was supposed to be based on height (using 80px) rather than padding. If you want to use padding can you just round to the nearest one, so either 16px or 24px? And remove the --spacing-20 var
| } | ||
| .mobile-menu > .button { | ||
| border-radius: 0; | ||
| font-size: 16px; |
There was a problem hiding this comment.
never write any CSS for fonts! Assign whichever is the right class based on the Figma, or just h1 or h2 tag.
app/newRoot.css
Outdated
| @@ -32,6 +32,7 @@ | |||
| --spacing-8: 8px; | |||
| --spacing-12: 12px; | |||
| --spacing-16: 16px; | |||
| --spacing-20: 20px; | |||
There was a problem hiding this comment.
remove this, sorry!
| @@ -22,6 +22,7 @@ const Button = ({children, action, tooltip, icon, className}: ButtonProps) => { | |||
| return ( | |||
| <button className={classes} onClick={action} data-tooltip={tooltip}> | |||
| {children} | |||
| {icon && icon} | |||
There was a problem hiding this comment.
just add it to the children, rather than adding an extra property just for this. Also, {icon && icon} is exactly the same as {icon} (I'm actually supprised that the linter didn't complain)
No description provided.