Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UI: Header component #13
UI: Header component #13
Changes from 13 commits
74ed827
9571039
be0b381
e7d6465
0e687e1
474a5f4
42aa0c1
533e129
4de9b63
c105104
a69046a
639785d
67b2549
f113c2e
3c9f8aa
838dcc3
3a23252
b9ed545
7329cef
5252f89
2f62eb9
d2ce507
7bb9859
3b161e4
764b0c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we use
146-20
value inml
? Where this value comes from? 🤔Looking at designs it looks like the navigation menu will always be in line with the actual page layout (from the left side). Please look at the screen below:
The red line shows the positioning of navigation bar to the page content. As you can see it's start in the same position on the left side. It looks similar in all pages.
Considering that we will probably use a
Container
for the page layout below (but correct me if I'm wrong) then maybe somehow we should use aContainer
at the navigation bar? Or maybe we could use a grid to properly do that? I don't have a quick solution for that, but we can discuss this issue more if needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be
142 - 20
. Fixed.As you can see on screenshot I added horizontal padding for
NavigationItems
to make it more accessible - easier to click by enlarging the area. To keep it pixel perfect the margin betweenLogo
andNavigationMenu
must be reduced by the item padding so spacing is now122px
of menu margin +20px
of item padding which results as142px
as it states in designs.I've added comment to explain the math behind the operation.
Ref commit: 3c9f8aa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced about this solution. It depende on how we want the menu and the content to behave on different resolutions. I would assume that when the width of the screen decreases, then the space between logo and navigation should decrease too so that we keep the navigation inline with the content (see my screenshot in the first comment).
For example, this is how out current tbtc implementation behaves when decreasing the width of the screen:
As you can see, the left space between the content and the left navigation decreases. I assume Bitcoin on Base view will behave the same so we would have to consider this when implementing the navigation. Otherwise it might a little bit off.
We can take a look at it later when we implement the layout. It will be easier to evaluate how it should behave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fonts do we want to load here? Is it something we want to address in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable to do it in separate PR to prevent blocking this PR.
I wasn't able to make it - not sure how Chakra theming works in terms of font setting.
https://discord.com/channels/893441201628405760/1181551917332185148/1181634992409944204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already applied font changes. Ref PR: #30