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

Article side menu #46

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Article side menu #46

wants to merge 3 commits into from

Conversation

bnaolu
Copy link
Contributor

@bnaolu bnaolu commented Nov 18, 2021

Spent a ton of time in the position-sticky world and I am putting my white flag up. A few things:

  • position-sticky for side menu not properly working on mobile (add as an issue to the repo if it can't be fixed?)
  • usa-current class applied only to the first jump link...should we include this or omit it entirely? It looks a bit odd visually either way, but I can't quite figure out how to make this state active for a specific id(#) in Jekyll 🤔

- built upon the original article template
- the side menu can be moved by swapping out the value in the 'side-menu-order' key
- updated the CDN in the head.html include
@bnaolu bnaolu requested a review from pglevy November 18, 2021 18:01
@@ -5,7 +5,7 @@
<meta name="description" content="{{ site.description }}">
<meta name="viewport" content="width=device-width, initial-scale=1">

<script src="https://cdnjs.cloudflare.com/ajax/libs/uswds/2.12.1/js/uswds.min.js" integrity="sha512-0I5hS86ISi9DRRJgArVIUFaI0fUpdxGppKIc6Dkl3NkdSNLwR2523QvMADILsfE1yK+nKbWuGDx+99+Fop3IgA==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/uswds/2.12.1/css/uswds.min.css" integrity="sha512-wICx8of+w4Nz/tYsj4B6rYxkrkgOH8zJ7bcyaShiM7jMPtDVZGcMQz7EDaBIqpxu9XGvtR7RKeSj3Zp5pGrISQ==" crossorigin="anonymous" referrerpolicy="no-referrer" />
<script src="https://cdnjs.cloudflare.com/ajax/libs/uswds/2.12.2/js/uswds.min.js" integrity="sha512-JZPbOlveyjzEUwcAJFmSQdxwz69XMfgQpXT5qqJ3jPwhyJu6FXeEcCzcpHgsV1kuj/5/VHgs1XyAoR3bmirXPQ==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should try to do changes that affect all templates in separate commits/PRs, so it's easier to follow that trial of what happened when. A feature/fix branch should have changes primarily related to that thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good to know! Thank you!

<img src="{{ page.hero-image }}" alt="{{ page.hero-image-alt }}" class="width-full">
<div class="grid-container">
<div class="grid-row grid-gap flex-align-start">
<div class="desktop:grid-col-3 tablet:order-{{ page.side-menu-order }} margin-y-3 position-sticky top-1">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did a few things to get this to work in Dev Tools. (You could also play around with bottom padding and maybe shadow to make it look a little nicer.)

  • Remove order (I think this was causing to appear on top in the layer order.)
  • Add bg color (so you don't see stuff behind it)
  • Add z-top to make sure the nav stays on top of the other content
  • Also did top-0 so content in background didn't poke through at top

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note if you need order to affect the layout at certain sizes, you might just have to make sure you have it set a particular way as default (small size).

{% for item in page.side-menu %}
<li class="usa-sidenav__item">
<!--Should we include the usa-current class?-->
<a href="#{{ item.id }}" class="{% if item.id == "understand-the-problem" %}usa-current{% endif %}">{{ item.name }}</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't there's a way to do this with Jekyll in this case because we're not linking to new pages, we're just linking to anchors on the current page. So you'd have to use some kind of JavaScript to dynamically update the HTML based on interaction with the page (e.g., clicking or scrolling).

We could just leave the static class in as an example. Probably don't need the template logic since it doesn't work in this case. If you wanted, you could play with some JS. My go-to would be addClass and removeClass with jQuery based on clicks.

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.

2 participants