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.

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

fixed responsiveness in header on the countries page #454

Merged
merged 6 commits into from
Jun 23, 2020

Conversation

NidhiKJha
Copy link
Contributor

Fixes #446

This pull request adds the responsiveness in mobile -view

  1. Desktop-view
    Screenshot from 2020-04-27 00-56-54

  2. Mobile-view
    Screenshot from 2020-04-27 00-56-35

@NidhiKJha
Copy link
Contributor Author

@sarathms @hellais please review my pull request. My implementation differs a bit from the mockup you provided. Please let me know if you want me to change it. Thank you :)

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

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

This is a great start. However, I think it would look better if we can align it with the mockups. Feel free to ask if you need help with anything.

To swap the position of the search input from right to left in the two layouts, you can use the order property of flexbox. I think I have used it in one other place in the codebase to achieve something similar for a mobile layout.

@@ -118,9 +118,9 @@ const StyledRegionLink = styled.a`
`

const RegionLink = ({ href, label }) => (
<Box px={3} py={4}>
<Box px={[1,3]} py={[2,4]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Vertical spacing py can go one step higher here.

Copy link
Contributor Author

@NidhiKJha NidhiKJha Apr 29, 2020

Choose a reason for hiding this comment

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

Sorry for the late follow up. I tried increasing the py but it was not going perfectly with the UI so I didn't change it.

@NidhiKJha
Copy link
Contributor Author

Hi @sarathms ! I have made changes in the positioning of the search but without using order.(But thanks for mentioning the use of order I learnt it :p) Please let me know if I have missed something. Thank You :)

@NidhiKJha
Copy link
Contributor Author

NidhiKJha commented Apr 29, 2020

In the mockups, there is a underlining in the Africa. I tried to add it. But I couldn't. This was my approach. Please let me know how should I approach :)

const Underline = styled.span
  display: block;
  height: 2px;
  background: ${props => props.theme.colors.blue5};
  width: ${props => props.active ? '100%' : '0px'};
`

const _RegionLink = ({ router, href, label }) => {
  const checkActive = href === router.asPath.replace('/countries', '')
  const [active, setActive] = React.useState(checkActive)
  console.log("test", href, active)

  return (
    <Box px={[1,3]} py={[2,4]}>
      <StyledRegionLink onClick={() => setActive(checkActive)} href={href}>
        <Text fontSize={[14, 20]}>{label}</Text>
        <Underline active={active}/>
      </StyledRegionLink>
    </Box>
  )
}

const RegionLink =  withRouter(_RegionLink) 

@NidhiKJha NidhiKJha requested a review from sarathms April 29, 2020 20:38
@sarathms
Copy link
Contributor

sarathms commented May 6, 2020

In the mockups, there is a underlining in the Africa

The approach you mentioned above is also good and that's how the NavBar links are styled. Alternatively, you can try hover styling with border-bottom inside StyledRegionLink but applied to its child Text
Something like this:

const StyledRegionLink = styled.a`
  ...
  & > ${Text}:hover {
     display: inline;
     border-bottom: 2px solid ${blue5 color from theme};
  }
`

The display: inline is crucial.

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @NidhiKJha. 👏 It wasn't trivial. I tried it and it looked quite close to the mockups. You seem to have got a hang of rebass 😸

I left a few comments to make improvements. You are right about the order. I didn't notice that the mockup placed the search input on the left in both layouts. I thought the change was only for mobile. My non-solution was overcomplicated! 😆

components/NavBar.js Outdated Show resolved Hide resolved
components/NavBar.js Outdated Show resolved Hide resolved
pages/countries.js Outdated Show resolved Hide resolved
@NidhiKJha
Copy link
Contributor Author

Hi @sarathms I have made all the changes suggested by you. Please review the pull request. Thanks :)

Copy link
Contributor

@sarathms sarathms left a comment

Choose a reason for hiding this comment

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

This looks good to merge. I fixed a few minor spacing issues and the jumping hover links. Thanks @NidhiKJha for working on this.

@sarathms sarathms merged commit 8d9e66d into ooni:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header on the countries page is not reponsive in mobile-view
3 participants