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

feat(website): Add text color token accessibility rating to the token page #28

Merged
merged 8 commits into from Aug 13, 2019

Conversation

@SiTaggart
Copy link
Collaborator

SiTaggart commented Aug 8, 2019

Add visual indication of the Accessibility rating of the text color tokens for small text.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.
@now

This comment has been minimized.

Copy link

now bot commented Aug 8, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://paste-git-feat-tokens-contrast-rating.twilio-dsys.now.sh

@@ -0,0 +1,14 @@
import styled from '@emotion/styled';

export const ScreenReaderOnly = styled.span<{}>`

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 12, 2019

Collaborator

Out of scope but I would love to make a page on this and basically break it out into:

  • Completely hidden (SR and UI)
  • SR Hidden (Only SR tech can't see it, but visually there (decorative icons, aria-hidden))
  • UI Hidden, but SR Visible

Maybe have one component that can be used for all variants:

<Visibility variant="none | screenreader-only | visible-only">

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 12, 2019

Author Collaborator

Interesting, we should explore this. I'm currently torn, I like the specificity of calling the component name out explicitly for its purpose, just to raise the awareness of Accessibility to engineers: "oh, there's a screen reader component". But I kinda like the idea of a visibility component too... 🤔

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 13, 2019

Collaborator

Yeah I'm torn on the name too but figured I'd just write it out and move on lol. Visibility means something specific in CSS, but it is rarely used so it should be fine. On the other hand, I don't like overloading terms like this but I do think this is a good name for it.


export const ScreenReaderOnly = styled.span<{}>`
position: absolute;
margin: -1px;

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 12, 2019

Collaborator

nit: while this is functional, could we do for correctness

margin-left: -1px;
margin-top: -1px;
height: 1px;
overflow: hidden;
clip: rect(0 0 0 0);
text-transform: none;

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 12, 2019

Collaborator

Why do we need this line? 🤔

@@ -44,7 +44,10 @@ export const Td = styled.td(props => ({
},
}));

export const Th = styled(Td)(props => ({
interface ThProps {

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 12, 2019

Collaborator

What's this for?

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 12, 2019

Author Collaborator

Typescript was complaining that width wasn't a valid prop. I can kinda see where it's going wrong in that the styled component is typed based on the element you give it. Seeing as this is a styled Td it may not be able to understand that it should extend the props from td element

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 13, 2019

Collaborator

I think you're right. I was able to get around it by doing

const getTdStyles = (props: {theme: {}}): {} => ({
  padding: `${themeGet('space.space40')(props)} ${themeGet('space.space30')(props)}`,
  verticalAlign: 'top',
  wordWrap: 'break-word',

  '&:nth-of-type(1)': {
    paddingLeft: themeGet('space.space50')(props),
  },

  '&:last-child': {
    paddingRight: themeGet('space.space50')(props),
  },
});

export const Td = styled.td(getTdStyles);

export const Th = styled.th(getTdStyles, props => ({
  textAlign: 'left',
  fontWeight: themeGet('fontWeights.fontWeightSemibold')(props),
}));

There oughta be an easier way...

This comment has been minimized.

Copy link
@SiTaggart

SiTaggart Aug 13, 2019

Author Collaborator

Is that in one of your PRs? But yeah that's the solution right there

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 13, 2019

Collaborator

Nope just tested it locally while reviewing this PR

let results: ColorCombosTypes[] = [];

const MINIMUMS: {aa: number; aaLarge: number; aaa: number; aaaLarge: number} = {
aa: 4.5,

This comment has been minimized.

Copy link
@TheSisb

TheSisb Aug 12, 2019

Collaborator

How did you settle on these values?

This comment has been minimized.

Copy link
@SiTaggart
@SiTaggart SiTaggart force-pushed the feat/tokens-contrast-rating branch from ed03a26 to 6c99746 Aug 13, 2019
@SiTaggart SiTaggart merged commit 74f12d1 into master Aug 13, 2019
6 checks passed
6 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
@SiTaggart SiTaggart deleted the feat/tokens-contrast-rating branch Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.