Navbar dropdown component#17
Conversation
marcus-ong-qy
left a comment
marcus-ong-qy
left a comment
There was a problem hiding this comment.
Do delete package-lock.json. And remember next time pull req add some screenshots ya
Do make the textboxes bigger, text looks cramped right now. Refer to the figma file!
Also, when mouseover text enlarge they overflow the dropdown, which is bad
In general not bad! Basic idea is there but just need refinements
| key={index} | ||
| text={item} | ||
| isActive={item.toLowerCase() === pageName || (item === 'Home' && pageName === '')} | ||
| isDropdown={item.toLowerCase() === 'about us'} |
There was a problem hiding this comment.
ok acceptable for now
|
|
||
| {dropdown && ( | ||
| <DropdownDiv> | ||
| {dropdownTitles.map((item, index) => ( |
There was a problem hiding this comment.
nice attempt to abstract the dropdown titles! should work for now
There was a problem hiding this comment.
but think what if we want to have multiple tabs on the navbar to have different dropdown menus? and how you store information such as which URL will each dropdown option direct to
| <NavItem | ||
| key={index} | ||
| text={item} | ||
| isDropdown={item.toLowerCase() === 'about us'} | ||
| href={item === 'Home' ? '/' : `/${item.toLowerCase()}`} | ||
| isInColumn={isInColumn} | ||
| /> |
There was a problem hiding this comment.
Hmm you should create a new component for dropdown item instead of just reusing the navbar NavItem component!
| if (isDropdown) { | ||
| return ( | ||
| <StyledNavItem | ||
| href={href ?? ''} | ||
| isActive={isActive} | ||
| fontType={h3} | ||
| onMouseEnter={onMouseEnter} | ||
| onMouseLeave={onMouseLeave} | ||
| > | ||
| <div>{text}</div> | ||
|
|
||
| {dropdown && ( | ||
| <DropdownDiv> | ||
| {dropdownTitles.map((item, index) => ( | ||
| <NavItem | ||
| key={index} | ||
| text={item} | ||
| isDropdown={item.toLowerCase() === 'about us'} | ||
| href={item === 'Home' ? '/' : `/${item.toLowerCase()}`} | ||
| isInColumn={isInColumn} | ||
| /> | ||
| ))} | ||
| </DropdownDiv> | ||
| )} | ||
| </StyledNavItem> | ||
| ) | ||
| } | ||
| if (isInColumn) { | ||
| return ( | ||
| <ColumnNavItemContainer href={href ?? ''} isActive={isActive} fontType={h3}> | ||
| {text} | ||
| </ColumnNavItemContainer> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Instead of putting rerturns within if else statement what you could do is:
- Abstract the components that you want to render on certain conditions only (i.e. the dropdown)
- Have only ONE return statement
- In the return statement, do something like
isDropdown && <Dropdown />. This will then only render the dropdown when the argumentisDropdownis set totrue
| @@ -0,0 +1 @@ | |||
| export const dropdownTitles = ['Text1', 'Text2', 'Text3', 'Text4'] | |||
There was a problem hiding this comment.
Hmm actually you can put this under the navTitles.ts file together
| const onMouseLeave = () => { | ||
| if (window.innerWidth < 960) { | ||
| setDropdown(false) | ||
| } else { | ||
| setDropdown(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
both is the same??????????
2 participants
No description provided.