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

refactor: Navbar: constrain max width to same width as the content. #1855

Merged
merged 4 commits into from
Dec 14, 2022

Conversation

jankoegel
Copy link
Contributor

@jankoegel jankoegel commented Dec 10, 2022

Related Rails app PR

HELP NEEDED / TODOs

  • review code
  • depending on responsiveness needs, consider moving the navigation entries ("websites" etc.) a bit more to the right so they are more centered instead of leaning left.
  • reviewer: test with an extension development setup (or pair with me / let me know how to set one up)
  • reviewer: test responsiveness (current extension's header looks as broken to me on smaller breakpoints as with these changes but I'm not sure!?)

Describe the changes you have made in this PR

  • Navbar: constrain max width to same width as the content.
  • reason: on desktop the navigation items in the top left and right corners were easy to miss. With this change, everything is closer together and easier to notice.

Type of change

  • UI improvement

Screenshots of the changes [optional]

Before

SCR-20221210-ugq

After

SCR-20221210-ug6

How has this been tested?

  • ⚠️ I only tested this with my browser's inspector. I don't have a development setup for the extension yet. Ok, I have the development setup for the extension now and everything looked good to me.

Reason: on desktop the navigation items in the top left and right
corners were easy to miss. With this change, everything is closer
together and easier to notice.
@github-actions
Copy link

github-actions bot commented Dec 10, 2022

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: Adam Fiscor (who recently dropped 1337 sats):

The future is bright. We just have a lot of work to do.

Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@bumi bumi requested a review from reneaaron December 12, 2022 08:39
+ remove superfluous <div>.
@bumi bumi added this to the v1.21 milestone Dec 12, 2022
  + let `.justify-between` do the work and don't define widths on the
dropdown and hamburger menus.
  + add `.lg:-ml-2` to `children` since proper centering looks a bit
    too much on the left (it's a human issue :).
Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

I think this is a good adjustment.

This looks a bit weird, but not to problematic I think:
image

@@ -7,16 +7,10 @@ type Props = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best reviewed in "split mode". I actually added a new wrapper div to constrain the content's width.
Since I also removed wrapper divs from <AccountMenu>, children and UserMenu it's a bit hard to see the changes in other views.

@jankoegel
Copy link
Contributor Author

I think this is a good adjustment.

This looks a bit weird, but not to problematic I think: image

yes, there is a weird space of about 100px display resolution around 1024px where the wallet icon has space on the left and is not flush with the content's left border. in the same space the negative margin on the children is a problem. Didn't investigate further since responsiveness is not a big focus. We could remove the negative margin, at full-res I felt it looks more centered with it than without.

@jankoegel
Copy link
Contributor Author

jankoegel commented Dec 14, 2022

I think this is a good adjustment.

This looks a bit weird, but not to problematic I think

hm, just checked again and it feels like i don't get the children that close to the wallet icon until a much smaller breakpoint on firefox.

@jankoegel
Copy link
Contributor Author

hm, just checked again and it feels like i don't get the children that close to the wallet icon until a much smaller breakpoint on firefox. Does the following GIF match what you're seeing?

looks the same as in Firefox for me on Chrome.

@jankoegel jankoegel merged commit db9beaf into master Dec 14, 2022
@jankoegel jankoegel deleted the refactor/header-max-width branch December 14, 2022 12:29
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.

3 participants