Mobile menu style fixes #602
Conversation
v2/src/layout/header.js
Outdated
| @@ -151,6 +151,30 @@ const Header = () => { | |||
|
|
|||
| const theme = data.site.siteMetadata.themeColours | |||
|
|
|||
| function MobileDropdown({ children, links, navLinkClass }) { | |||
There was a problem hiding this comment.
Why introduce a copy of the existing MobileDropdown instead of fixing the existing one? As far as I can tell, the one in components/dropdownButtons/dropdown is no longer used now.
There was a problem hiding this comment.
For sure, cleaned that up. I moved it to the header so I could trigger the collapse state of the whole menu when language is selected. Otherwise could only close the language menu
There was a problem hiding this comment.
The fact you need to track the dropdown state in the header rather than the dropdown is a good indication that there is already too much in the header. You are making the problem worse.
Ultimately, why is this the header's job to know if the dropdown is mobile or not or what? The header ask for a dropdown, it's up to the drop down component to sort this mess out.
There was a problem hiding this comment.
Good points. I've refactored a bit and separated the components
v2/src/layout/header.js
Outdated
| @@ -151,6 +151,30 @@ const Header = () => { | |||
|
|
|||
| const theme = data.site.siteMetadata.themeColours | |||
|
|
|||
| function MobileDropdown({ children, links, navLinkClass }) { | |||
There was a problem hiding this comment.
The fact you need to track the dropdown state in the header rather than the dropdown is a good indication that there is already too much in the header. You are making the problem worse.
Ultimately, why is this the header's job to know if the dropdown is mobile or not or what? The header ask for a dropdown, it's up to the drop down component to sort this mess out.
v2/src/layout/header.js
Outdated
| const [currentUSDPrice, setCurrentUSDPrice] = useState("-") | ||
| const updateBchPrice = () => { | ||
| axios.get(bchPriceApi).then(response => { | ||
| if (response.data && response.data.USD) { | ||
| setCurrentUSDPrice(response.data.USD) | ||
| setCurrentUSDPrice(response.data.USD.toFixed(2)) |
There was a problem hiding this comment.
This has nothing to do with the patch at hand.
v2/src/layout/header.js
Outdated
| )} | ||
| </div> | ||
| <NavItems navBarItems={navBarItems} /> | ||
| <MobileNavItems navBarItems={navBarItems} /> |
There was a problem hiding this comment.
Either this is some kind of generic component that generate a navbar, in which case it's hard to justify this components having to deal with the fact that navbar for mobile or desktop des things differently, or it is not generic, and then it is hard to justify that this is the pieces of code that needs to handle what is in the navbar.
You really need to think about what is responsible of what and split things accordingly.
There was a problem hiding this comment.
Can you help me understand this? I would say the header component is not generic. So....there shouldnt be seperate components for the NavItems and MobileNavItems? Or both of those should be directly in the header?
There was a problem hiding this comment.
You need to decide what the responsibility of each piece of code is and split things accordingly.
|
Are you talking about this PR or the nav components? |
|
I'm talking about the fact that this very PR now does a ton of stuff. It can be broken down. |
|
Okay for sure. It got a bit carried away because the whole header component needed rearragning to achieve the original goal of improving mobile nav UX. I'll break it up |
|
Don't be worried, this is a normal part of the process. You start pulling the string, a whole ball of stuff comes with it, you deal with it. Now is the time to break it up. For instance, there are refactoring in there - changes in the structure of the code that do not change the behavior. You might want to extract these in their own patch, and then you can apply the behavioral changes on top of it. |
|
Okay broke these changes out into other PRs. I'll close this PR to avoid confusion |
No description provided.