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

WWW-960: mobile nav top link color #2314

Merged
merged 3 commits into from
Sep 10, 2021

Conversation

remydenton
Copy link
Collaborator

@remydenton remydenton commented Sep 9, 2021

Jira

https://pegadigitalit.atlassian.net/browse/WWW-960

Summary

Makes unselected top-level mobile links black for consistency

Details

See screenshots in the "visual changes" section below. All other colors should be unchanged.

How to test

See screenshots in the "visual changes" section below. All other colors should be unchanged.

Release notes

Visual changes

In the global nav, top-level links are now black instead of dark navy when not selected. For example, on https://boltdesignsystem.com/pattern-lab/?p=components-page-header-example-main-site-with-subnav

Before:
Screen Shot 2021-09-10 at 10 44 12 AM

After:
Screen Shot 2021-09-10 at 10 44 24 AM

This rule is already overridden for desktop links in this line:
https://github.com/boltdesignsystem/bolt/blob/37832407b915bc1f4be32851d1d1441799d1866e/packages/components/bolt-page-header/src/_page-header-desktop.scss#L315

Mobile links are the only ones it affects, and the only ones that are
reported as having a problem in WWW-960.
This is only necessary because the changes in the following commit have
greater specificity than the defaults for hover and focus states defined
in the `bolt-page-header-action-trigger` mixin.

3167fb8
@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Sep 9, 2021
Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

Page header is very complicated. Your change affected some other elements. I need a closer look tomorrow.

@colbytcook colbytcook temporarily deployed to feature/WWW-960-mobile-nav-top-link-color--branch-preview September 9, 2021 22:43 Inactive
@mikemai2awesome
Copy link
Collaborator

All set.

@remydenton
Copy link
Collaborator Author

With the latest changes, top level links have a different color in focus state (black)...
Screen Shot 2021-09-10 at 11 45 15 AM

... than other links in focus state (dark navy)
Screen Shot 2021-09-10 at 11 46 14 AM

I actually don't see a good reason to have an [aria-expanded='true'] style for mobile links, since they have identical styling to other links, hence my commit comment in a2e752.

At the end of the day, I don't really care as long as stakeholders are happy, and you're the original author @mikemai2awesome, so your call.

@mikemai2awesome
Copy link
Collaborator

mikemai2awesome commented Sep 10, 2021

@remydenton They are not both in expanded state (one is acting as a heading and expanded, the other is an item and not expanded), so the color should be kept consistent (expanded state is black).

@remydenton
Copy link
Collaborator Author

remydenton commented Sep 10, 2021

They are not both in expanded state (one is acting as a heading and expanded, the other is an item and not expanded), so the color should be kept consistent (expanded state is black).

I'm aware they're not both expanded. If you're trying to show that they're different (in terms of expanded/not expanded), why do they look the same when not in focus state?

IMO, they should look identical, unless you want to distinguish between expanded and not-expanded links, in which case they should be styled differently both when focused and not.

Pretty sure the original PR makes more sense, unless there are other situations I'm not aware of.

@mikemai2awesome
Copy link
Collaborator

@remydenton They shouldn't look the same intially. I intended headlines to take on headline color, links to take on link color, but I'm forced to change them to black which makes no sense.

@remydenton
Copy link
Collaborator Author

I'm forced to change them to black which makes no sense.

Fair, I think that's probably the root cause of why it feels weird to me. I'm fine with this as-is, but let me have stakeholders take a look at the Pattern Lab demo link first so that we don't run the risk of yet more change requests 😄 .

@colbytcook
Copy link
Contributor

@mikemai2awesome @remydenton it looks like Ross responded to the JIRA question, we should be good to merge this one!

@mikemai2awesome mikemai2awesome merged commit 8e399d6 into master Sep 10, 2021
@mikemai2awesome mikemai2awesome deleted the feature/WWW-960-mobile-nav-top-link-color branch September 10, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants