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

fix(docs-infra): improve focus styles in topnav and footer #33255

Closed

Conversation

sjtrimble
Copy link
Contributor

Fixes #33239

Improved focus styles for topnav and footer including visible color.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Topnav:
focus-topnav-old

Footer:
focus-footer-old

Issue Number: #33239

What is the new behavior?

Topnav:
focus-topnav-new

Footer:
focus-footer-new

Does this PR introduce a breaking change?

  • Yes
  • No

@sjtrimble sjtrimble requested a review from a team as a code owner October 18, 2019 17:54
@sjtrimble sjtrimble self-assigned this Oct 18, 2019
@sjtrimble sjtrimble added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs-infra labels Oct 18, 2019
@ngbot ngbot bot added this to the needsTriage milestone Oct 18, 2019
@mary-poppins
Copy link

You can preview 735eefd at https://pr33255-735eefd.ngbuilds.io/.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Hi @sjtrimble - good to see you back to work.
When I look at the last preview URL, it doesn't look the same as your "new behaviour" animations.
Are they out of date?

Apart from that the focussing looks much better than before.

Also, perhaps out of range for this PR, but it is weird that if the browser is the same width as in your example animations, focussing the search box actually make it go smaller. Can we fix that?

@sjtrimble
Copy link
Contributor Author

@petebacondarwin Heya! Weird, to me the preview URL and the GIF look the same. What is different?

That is super weird about the search box! I'll open an issue for it. It seems to happen in a smaller screen size.

@sjtrimble
Copy link
Contributor Author

@gkalpak here's the PR that needs your help with getting active state/class on the topnav links for styling.

@gkalpak
Copy link
Member

gkalpak commented Oct 22, 2019

@sjtrimble, let's fix the focus styles on this PR and I'll create a separate one for highlighting active tobbar nav links. EDIT: Created #33351.

As Pete mentioned, the styling on the preview looks different than your gifs 😁
Here is what it looks like on my laptop:

focus-styles

The main (undesirable) differences I see are:

  • There is no space between the logo and the focus outline.
  • The topbar nav links still have the lighter background color (in addition to the outline) when focused.
  • There is too much space between the Twitter and GitHub logos and their focus outline (and each outline intersects with the other logo).

Does it look different on your machine?

@sjtrimble
Copy link
Contributor Author

@gkalpak Super strange .. yeah, it doesn't look like that on my machine. I'm using a Chrome browser on my Mac both on my local version and in the PR preview link 🤔

I checked on Safari and it changes the color there for some of the outlines which is also strange ..
Screen Shot 2019-10-22 at 11 10 37 AM
Screen Shot 2019-10-22 at 11 10 22 AM
Screen Shot 2019-10-22 at 11 10 12 AM

gkalpak added a commit to gkalpak/angular that referenced this pull request Oct 23, 2019
@gkalpak gkalpak added this to MERGE in docs-infra Dec 10, 2019
@gkalpak gkalpak removed this from MERGE in docs-infra Dec 10, 2019
@gkalpak gkalpak added state: WIP target: patch This PR is targeted for the next patch release type: bug/fix and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 10, 2019
@gkalpak gkalpak added this to IN PROGRESS in docs-infra Dec 10, 2019
@sonukapoor
Copy link
Contributor

@gkalpak I have a fix for this ready. I will discuss it with you offline.

@sonukapoor sonukapoor mentioned this pull request Jan 3, 2020
14 tasks
@sonukapoor
Copy link
Contributor

@sjtrimble / @gkalpak Please see followup PR with a fix:

#34634

@googlebot

This comment has been minimized.

@gkalpak
Copy link
Member

gkalpak commented Jan 17, 2020

@sjtrimble, could you take a look at my fixup commit and verify you are happy with the changes and they look OK on macOS?

@gkalpak
Copy link
Member

gkalpak commented Jan 17, 2020

I pushed another fixup commit to fix CI.

@mary-poppins
Copy link

You can preview 9073352 at https://pr33255-9073352.ngbuilds.io/.

@sonukapoor

This comment has been minimized.

@gkalpak

This comment has been minimized.

@sonukapoor

This comment has been minimized.

@gkalpak

This comment has been minimized.

outline-color: rgba($white, 0.8);
outline-offset: 2px;
// `outline-offset` is not applied on Chrome on Windows, if `outline-style` is `auto.
outline-style: solid;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to add outline-width: 1px so it doesn't look excessive.

Screen Shot 2020-02-18 at 2 57 35 PM

Copy link
Member

Choose a reason for hiding this comment

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

Done.

outline-color: $focus-outline-ondark;
outline-offset: 4px;
// `outline-offset` is not applied on Chrome on Windows, if `outline-style` is `auto.
outline-style: solid;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to add outline-width: 1px so it doesn't look excessive.

Screen Shot 2020-02-18 at 3 01 52 PM

Copy link
Member

Choose a reason for hiding this comment

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

Done (and also for the social icons).

@gkalpak
Copy link
Member

gkalpak commented Feb 19, 2020

Rebased on master and explicitly set the outline-width to 1px. Thx, @sjtrimble!
I am marking this for merging. If there are more tweaks needed, we can make them in a follow-up PR.

@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 19, 2020
@mary-poppins
Copy link

You can preview 02ffecd at https://pr33255-02ffecd.ngbuilds.io/.

@gkalpak
Copy link
Member

gkalpak commented Feb 19, 2020

(The payload size failure in test_aio_local will be fixed with #35538.)

@mary-poppins
Copy link

You can preview a088d00 at https://pr33255-a088d00.ngbuilds.io/.

alxhub pushed a commit that referenced this pull request Feb 19, 2020
@alxhub alxhub closed this in 1997b86 Feb 19, 2020
gkalpak added a commit to gkalpak/angular that referenced this pull request Feb 20, 2020
gkalpak added a commit to gkalpak/angular that referenced this pull request Feb 20, 2020
mhevery pushed a commit that referenced this pull request Feb 20, 2020
mhevery pushed a commit that referenced this pull request Feb 20, 2020
@gkalpak gkalpak moved this from IN PROGRESS to MERGE in docs-infra Mar 3, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
Development

Successfully merging this pull request may close these issues.

Top Menu highlight wrong item after click on browser back button
6 participants