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

Jump to bottom

Update page.scss - Add Sticky Header/Fix Alignment #895

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update page.scss - Add Sticky Header/Fix Alignment #895

wants to merge 2 commits into from

Conversation

2140data
Copy link

Introduce a Sticky Header and Fix NavBar/Logo Alignment with 3 small CSS edits

Results: Adds Sticky Header and also fixes alignment of NavBar so it aligns with the Site Logo

Steps Taken:

  1. Created Sticky Header: Added position:sticky; and top:0; to .navigation-wrapper on page.scss
  2. Fixed NavBar Alignment: Edited .nav on page.scss from padding-top:1.5em; to padding-top:1.2em;
  3. Tested and verified locally as working

@2140data
Update page.scss - Add Sticky Header/Fix Alignment
4a4784f 
Add Sticky Header and also Fix Alignment of NavBar so it aligns with the Site Logo
@katesalazar
Copy link
Contributor

katesalazar commented Aug 23, 2022 via email

@2140data
Copy link
Author

2140data commented Aug 23, 2022 •
edited
Loading

On desktop with padding-top: 1.2em :

header

On smaller width with padding-top: 1.2em :

header-smaller

Here's a look at the Sticky header:

header-sticky-2

@mikeobank
Copy link
Contributor

@2140data
Copy link
Author

Yes, position:sticky has wide support (except IE (of course)), but it has built-in graceful degradation... if sticky header is not supported, it just stays static in the source (works as it does currently).

I am not seeing it broken on mobile, but I do see the flagged z-index issue (exists independently of this PR and could be fixed on its own).

Here is what I see on mobile:
Screen Shot 2022-08-24 at 8 59 09 AM

I also have a script for a better mobile alignment:
Screen Shot 2022-08-24 at 9 08 28 AM

@katesalazar
Copy link
Contributor

katesalazar commented Aug 24, 2022 via email

@2140data
Copy link
Author

there "smaller" looks better with the current padding-top, more elegantly aligned with the rest of the line, don't you think?
…
On Tue, Aug 23, 2022 at 10:17 PM Ant @.> wrote: On desktop with padding-top: 1.5em : [image: header] https://user-images.githubusercontent.com/72945059/186256427-67262ec5-ffac-4296-be7f-400e37b82e42.png On smaller width with padding-top: 1.5em : [image: header-smaller] https://user-images.githubusercontent.com/72945059/186256601-3926689c-8ad9-45e6-851e-8ebb6d57f018.png Here's a look at the Sticky header: [image: header-sticky] https://user-images.githubusercontent.com/72945059/186257383-a5c1d1a3-8da7-4058-9fcb-51142a202b09.png — Reply to this email directly, view it on GitHub <#895 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMRS4WYF7SCW3S4P2BHOIN3V2UWUNANCNFSM57MDZVZA . You are receiving this because you commented.Message ID: @.>

Yes, I agree it does align properly when screens are smaller and the NavBar wraps to the second line... but is that the default view? On desktop, the NavBar appears misaligned. We could make it a true responsive header, align the NavBar and Logo properly across all scenarios, and add a couple simple media queries to handle the display on all screensizes.

--

Mid-Comment Update... I just went to make a screenshot of the desktop view and realized the alignment issue is actually on the logo, not the NavBar... the NavBar is properly aligned in the middle of the Navigation Area, but the logo is closer to the top than the bottom. It looks like Site-Name has padding-top: 1.15em; but padding-top: 1.4em; or padding-top: 1.5em; provides true alignment. Could use a media query to keep the alignment true at all breakpoints...

Screen Shot 2022-08-24 at 1 57 10 PM

.

@mikeobank
Copy link
Contributor

I am not seeing it broken on mobile, but I do see the flagged z-index issue (exists independently of this PR and could be fixed on its own).

  • Was checking a 320 x 480 pixel view. Probably an edge case.
  • I do agree with fixing the z-index issues on all the other elements in a different PR. That would be cleaner than adding an arbitrarily large z-index to the header. That PR would be blocking getting this PR merged though.

@mikeobank
Copy link
Contributor

  • I do agree with fixing the z-index issues on all the other elements in a different PR. That would be cleaner than adding an arbitrarily large z-index to the header. That PR would be blocking getting this PR merged though.

It's probably not a z-index issue, but position: relative; instead. Something like this can fix it on the download page: master...mikeobank:bitcoincore.org:remove-image-position-relative. Not thoroughly tested.

@2140data
Copy link
Author

  • I do agree with fixing the z-index issues on all the other elements in a different PR. That would be cleaner than adding an arbitrarily large z-index to the header. That PR would be blocking getting this PR merged though.

It's probably not a z-index issue, but position: relative; instead. Something like this can fix it on the download page: master...mikeobank:bitcoincore.org:remove-image-position-relative. Not thoroughly tested.

Nice! Removing position: relative; from the download images and magnet link would be a good fix. On the other hand, since it is truly a position issue and not a z-index issue, then .navigation-wrapper can be set to z-index: 1; or something low. Preliminary tests worked well with z-index:1.

@mikeobank
Copy link
Contributor

.navigation-wrapper can be set to z-index: 1;

I agree

@katesalazar
Copy link
Contributor

katesalazar commented Aug 25, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@2140data @katesalazar @mikeobank