Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Show outline on keyboard nav and style outline to match theme#240

Merged
amiller-gh merged 4 commits into
nodejs:masterfrom
kevjin:style-outline
May 26, 2019
Merged

Show outline on keyboard nav and style outline to match theme#240
amiller-gh merged 4 commits into
nodejs:masterfrom
kevjin:style-outline

Conversation

@kevjin

@kevjin kevjin commented Apr 13, 2019

Copy link
Copy Markdown
Contributor

Description

This PR partially reverts the changes made in #224 to comply with a11y on outlines.

Outlines will display during keyboard navigation (ex. tabbing) and will hide on mouse clicks.

Outlines have also been styled using nodejs.dev's brand colors, for better visual appeal.

Default vs custom style comparison

Screen Shot 2019-04-12 at 9 38 43 PM

vs.

Screen Shot 2019-04-12 at 9 38 31 PM

Resolves #239

@codecov-io

codecov-io commented Apr 13, 2019

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@df0abde). Click here to learn what that means.
The diff coverage is 45.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #240   +/-   ##
=========================================
  Coverage          ?   40.07%           
=========================================
  Files             ?       22           
  Lines             ?      272           
  Branches          ?       61           
=========================================
  Hits              ?      109           
  Misses            ?      162           
  Partials          ?        1
Impacted Files Coverage Δ
src/components/layout.tsx 2.77% <0%> (ø)
src/util/outlineOnKeyboardNav.ts 53.84% <53.84%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df0abde...fa996d0. Read the comment docs.

@ahmadawais

ahmadawais commented Apr 15, 2019

Copy link
Copy Markdown
Member

@kevjin Feel free to run the following command to build a staging demo whenever there are visual changes to be tested.

Just type the following in a comment. It means Google Cloud Build Run.

/gcbrun

@ahmadawais

Copy link
Copy Markdown
Member

/gcbrun

@BeniCheni

Copy link
Copy Markdown
Contributor

Per /gcbrun, just friendly note that In a previous PR (#222), I inquired about gcbrun v.s. required access. It might be collaborator-only to build a staging given this comment(#222 (review)), but not 💯 sure:

Can I try /gcbrun too? Not sure how the access stuff works.

I wasn't not too sure, either. But considering that your comment did not trigger a build, I think you need to be a collaborator to do so.

Though, still haven't since the staging URL shown from @ahmadawais's previous gcbrun attempt though, and he's a member of the project. 🤔

@ahmadawais

Copy link
Copy Markdown
Member

/gcbrun

@kevjin

kevjin commented Apr 21, 2019

Copy link
Copy Markdown
Contributor Author

@ahmadawais weird that /gcbrun still didn't work. Can we try one more time, or maybe reset its configuration for the project (not sure how it's set up)

@kevjin

kevjin commented Apr 21, 2019

Copy link
Copy Markdown
Contributor Author

/gcbrun

1 similar comment
@ahmadawais

Copy link
Copy Markdown
Member

/gcbrun

@ahmadawais

Copy link
Copy Markdown
Member

@MylesBorins can you help here? The /gcbrun is not posting the staging link

@MylesBorins

MylesBorins commented May 15, 2019

Copy link
Copy Markdown
Contributor

github actions doesn't run for PRs from a fork which is why the URL hasn't posted

The URLs are deterministic though

staging example is https://storage.googleapis.com/staging.nodejs.dev/fa996d0/index.html

@ahmadawais ahmadawais left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! 🔥

@amiller-gh amiller-gh merged commit bd6c20b into nodejs:master May 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restyle focus block with nodejs colors

6 participants