Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Mobile menu style fixes #602

Closed
wants to merge 7 commits into from
Closed

Mobile menu style fixes #602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e7d0af6
style edits
johnkuney Sep 16, 2020
34135a0
remove mobile dropdown
johnkuney Sep 16, 2020
b96864c
remove uneeded usestate
johnkuney Sep 16, 2020
c900241
clean up
johnkuney Sep 16, 2020
6f27d51
remove unrelated patch for price ticker decimal places
johnkuney Sep 17, 2020
7c688a7
navbar
johnkuney Sep 18, 2020
32c770a
consolidate navbar
johnkuney Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions v2/src/layout/announcement-bar.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@
.accordionTitle {
margin-right: 0;
padding: 10px 0;
font-size: 12px;
display: inline-block;
}

.accordionTitle svg {
Expand Down Expand Up @@ -136,4 +138,9 @@
margin-right: 0;
width: 100%;
}

.accordionSection {
padding: 0px 10px;
text-align: center;
}
}
28 changes: 26 additions & 2 deletions v2/src/layout/header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import logo from "assets/images/bitcoin-cash-logo-white-small.png"
import { useStaticQuery, graphql } from "gatsby"
import headerStyles from "./header.module.scss"
import AnnouncementBar from "./announcement-bar.js"
import Dropdown, { MobileDropdown } from "components/dropdownButtons/dropdown"
import Dropdown from "components/dropdownButtons/dropdown"
import LivePriceWidget from "components/liveprice/live-price-widget"
import axios from "axios"
import fbt from "fbt"
Expand Down Expand Up @@ -151,6 +151,30 @@ const Header = () => {

const theme = data.site.siteMetadata.themeColours

function MobileDropdown({ children, links, navLinkClass }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points. I've refactored a bit and separated the components

const [expanded, setExpanded] = useState(false)
return (
<div onClick={() => setExpanded(!expanded)}>
<div className={navLinkClass}>{children}</div>
{expanded && (
<div>
{links.map(dropdownLink => (
<Link
className={headerStyles.mobileNavLinkLanguage}
key={dropdownLink.text}
to={dropdownLink.href}
localize={dropdownLink.localize}
onClick={() => setIsActive(!isActive)}
>
{dropdownLink.text}
</Link>
))}
</div>
)}
</div>
)
}

return (
<>
<AnnouncementBar />
Expand Down Expand Up @@ -218,7 +242,7 @@ const Header = () => {
style={
isActive
? {
height: "calc(100vh - 100px)",
height: "auto",
backgroundColor: theme.primary_dark,
}
: null
Expand Down
13 changes: 12 additions & 1 deletion v2/src/layout/header.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,23 @@ header {
}

.mobileNavLink {
padding: 30px 20px;
padding: 15px 20px;
color: white;
border-bottom: 1px solid #ffffff20;
display: flex;
}

.mobileNavLinkLanguage {
padding: 15px 0 15px 40px;
color: white;
border-bottom: 1px solid #ffffff20;
display: flex;
}

.mobileNavLink svg {
width: 12px;
}

@media only screen and (max-width: 1100px) {
.headerLinks {
display: none;
Expand Down