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

Navigation: Add "Log in" link to local nav #708

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Navigation: Add "Log in" link to local nav #708

merged 1 commit into from
Sep 11, 2024

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Sep 10, 2024

This updates the Pattern Directory to include the "Log in" link in the local navigation across the site. The link only appears when you're logged out, once logged in, you can manage your account/login status in the admin bar. Once logged in, the "My patterns" link replaces the "Log in" link.

This site already has no admin bar when logged out.

See WordPress/wporg-mu-plugins#647

Screenshots

Logged out Logged in
Screen Shot 2024-09-10 at 15 44 32 Screen Shot 2024-09-10 at 15 46 22

How to test the changes in this Pull Request:

  1. Be logged out
  2. There should be a "log in" link
  3. Clicking it and signing in should redirect you back to the page you were just on
  4. The "log in" link should not appear
  5. You should see the admin bar

cc @WordPress/meta-design

@ryelle ryelle added the [Component] Theme The frontend of the pattern directory, pattern lists UI label Sep 10, 2024
@ryelle ryelle self-assigned this Sep 10, 2024
@StevenDufresne
Copy link
Collaborator

I don't want to go against the momentum, but it's weird to me that the login link disappears and isn't replaced with my user account image or link to my account.

Apart from that, the code looks good. The way we have built our subnav will force duplicate code across all the themes. We may want to consider adding a filter or creating a block for this longer term.

Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

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

Works for me 💯

The way we have built our subnav will force duplicate code across all the themes. We may want to consider adding a filter or creating a block for this longer term.

Feels like the local nav could probably add this link by default, maybe with a block attribute to disable.

@jasmussen
Copy link

I don't want to go against the momentum, but it's weird to me that the login link disappears and isn't replaced with my user account image or link to my account.

I wouldn't be opposed to future explorations that add profile management to that toolbar. We might even be able to hide the adminbar for most user roles other than editor and admin. In that vein, I think of this as the first step, which will fix an important layout shift that happens currently to every visitor to the site that isn't logged in, and never will be, which is arguably the majority. I believe @fcoveram even has some mockups for further profile management; just important to not block this important first step.

@ryelle
Copy link
Contributor Author

ryelle commented Sep 11, 2024

Feels like the local nav could probably add this link by default, maybe with a block attribute to disable.

I thought about trying something like that, but all the themes use different menu slugs, and it's not requested for all the sites, so I couldn't auto-add it. An attribute could work, but some links need the has-separator class and some don't… overall it seemed easier to drop it in to each theme.

If we do come back to add in profile management (which, IMO, would need a lot of design and UX research to replace the admin bar, otherwise we'll have the same "layout jump" issue as today), we can iterate on this. For example, we might want a separate block for those user controls, not part of the nav menu.

@ryelle ryelle merged commit 7052381 into trunk Sep 11, 2024
3 checks passed
@ryelle ryelle deleted the add/log-in-link branch September 11, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Theme The frontend of the pattern directory, pattern lists UI
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants