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

[FEATURE] Add Link component to define link styles #129

Open
31 tasks
oxaudo opened this issue Nov 21, 2018 · 2 comments
Open
31 tasks

[FEATURE] Add Link component to define link styles #129

oxaudo opened this issue Nov 21, 2018 · 2 comments

Comments

@oxaudo
Copy link
Member

oxaudo commented Nov 21, 2018

Component Name

Link

Zeplin link

https://app.zeplin.io/project/5acd19ff49a1429169c3128b/screen/5afc758c09a18d3d0f176edd

Product team

all

Design lead

@nicoleyeo

Checklists

Design

For Tokens

Color, type, spacing, icons, etc

  • Is it serving a purpose? (is it needed or just purely decorative?)

  • Can it be merged with any existing styles on the site?

  • Has it been pressure-tested in all instances?

  • Does it meet accessibility standards? (min type size, color contrast)

For Elements

Buttons, links, loaders, inputs, dropdowns, etc

  • Is it using up-to-date tokens from above?

  • Have all states been captured? (hover, selected, disabled, focused, normal, thinking, errors)

  • Have all transitions/animations been defined?

  • Is it useable? (Considering touch targets, screen sizes)

  • Is it consistent with the rest of the visual system? (Corner radius, stroke weight, form, shadow, opacity, etc)

  • Does it meet accessibility standards? (is it keyboard navigable, does it have required accessibility markup)

  • Does it render and function properly in Artsy supported browsers?

For Components

Nav, modules, modals, cards, etc

  • Is it using up-to-date elements and tokens from above?

  • Does it communicate clearly? (Copy, writing style/tone)

  • Is it flexible? (Internationalization, text wrapping)

  • Is it logical from a UX perspective? Does it follow paradigms set on the rest of Artsy?

  • Has responsive behavior at all breakpoints been defined?

  • Have all transitions/animations been defined?

  • Does it meet accessibility standards?

  • Does it render and function properly in Artsy supported browsers?

Engineering

Accessibility

  • Do all images and multimedia have alt or title tags?

  • Are semantic elements used appropriately (nav, button, etc)?

  • Are new components keyboard-navigable?

  • Are hover interactions available by other means?

  • Are aria- attributes included where appropriate?

  • Has a Chrome Devtools accessibility audit been performed?

Compatibility

Has the new component been reviewed in Browserstack:

  • Desktop Chrome

  • Desktop Edge

  • Desktop Safari

  • Desktop Firefox

  • Android

  • iOS

@oxaudo oxaudo changed the title NEW: Add Link component to define link styles [FEATURE] Add Link component to define link styles Nov 21, 2018
@jonallured
Copy link
Member

I'm not sure what else to do with this feedback, so I'm going to put in here - lmk if there's a more appropriate place!! ❤️

While working on the preview for collections, @pepopowitz and I have run into some difficulty using Link - here's the comp we're working with:

https://app.zeplin.io/project/5aabc5d2e01881c81138e58e/screen/5c5cadd021ae342cd2903443

screen shot 2019-02-14 at 9 50 24 am

We've got a Box with a background image with a Link inside that wraps a couple Serif elements. The text should be white. The comp doesn't specify what should happen on hover, but let's assume no underline. What we end up with looks like this:

screen shot 2019-02-14 at 9 52 59 am

It's sorta hard to see, but the hover is underlining and doing so in black. And it behaves this way because we've used the noUnderline property. This property, however, simply means draw the underline only on hover. In order to never have an underline, you actually have to specify a color for the link.

I've got a commit on a fork of palette that demonstrates this in the docs for Link:

jonallured@9f21771

screen shot 2019-02-14 at 10 00 47 am

Here's what I think needs to be addressed:

  • When you want to specify a color for a link, it's problematic that the current spec calls for hover to be black - maybe black isn't appropriate for your use-case!
  • The noUnderline prop is misleading - what it means as implemented is "don't underline until hover". If you never want an underline it should not be used, instead you can provide a color? That seems like a strange API.

Ok that's my feedback, hope this helps and I'm happy to talk through this if anything is confusing!! 🤗

@oxaudo
Copy link
Member Author

oxaudo commented Feb 19, 2019

All great points here. I think those all are design questions. The current link was built to the spec above which obviously did not seem to consider 'black on black' case.

The 'noUnderline` prop was also taken directly from zeplin spec: https://app.zeplin.io/project/5acd19ff49a1429169c3128b/screen/5afc758c09a18d3d0f176edd

I agree that it is a bit misleading this name....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants