feat(link): introduce inline variant #3859

asudoh · 2019-08-29T09:18:06Z

Changelog

New

Inline link variant.

Testing / Reviewing

Testing can be done by looking at the inline link, open Chrome DevTools, go to Elements tab, select the <a>, and click on :hov button and change :active, etc. states.

Fixes carbon-design-system#3228.

netlify · 2019-08-29T09:28:30Z

Deploy preview for carbon-components-react failed.

Built with commit a23b1dd

https://app.netlify.com/sites/carbon-components-react/deploys/5d7a299af2693100085c6fe9

netlify · 2019-08-29T09:30:50Z

Deploy preview for the-carbon-components ready!

Built with commit a23b1dd

https://deploy-preview-3859--the-carbon-components.netlify.com

netlify · 2019-08-29T09:32:55Z

Deploy preview for carbon-elements ready!

Built with commit a23b1dd

https://deploy-preview-3859--carbon-elements.netlify.com

emyarod

looks good to me pending design review

tw15egan

I'm seeing hover styles applied when the disabled class is active.

shixiedesign

Two comments:

Disabled link has hover state (both vanilla & react):

Focus state should not have underline

I noticed this is a current focus state on default link too. If this needs to be a separate issue that's okay too.

Current:

Should be:

vpicone

@shixiedesign Is it intentional that disabled and visited have no underline?

Just like the only marker of a link being blue is insufficient, the same holds true for light grey and purple.

It is still a link even though it's been visited or disabled. For a11y purposes, there should be a second indicator of it being a link other than just the color.

vpicone · 2019-08-30T01:23:54Z

packages/components/src/components/link/_link.scss

+  .#{$prefix}--link--inline {
+    text-decoration: underline;
+
+    &:hover {


The hover color is incorrect in the normal link component above (link-01 as opposed to hover-primary-text) src. We should fix it there rather than having it only be correct for the inline style.

Per our style page, the default link component should also have a visited style that's being added here. It should instead be added to the regular link component unless that distinction is intentional @shixiedesign

shixiedesign · 2019-08-30T16:03:52Z

@vpicone I think you might be right. We want to keep the "variant" styling consistent between default & visited states so they are recognizable as the same component. So @asudoh if you don't mind, I updated the spec for inline link. From what I see at the moment, inline link's visited state currently have an underline, so we are all good there. Only need to add underline to disabled state.

Another bug I noticed: our default link currently don't have visited state. It should gain the purple color, but no underline, to stay consistent with its default state.

Thanks for fixing the underline on focus!

asudoh · 2019-09-02T03:02:08Z

Thanks the team for the input and especially @shixiedesign for taking time for updating the spec. Incorporated the spec update.

shixiedesign

I see the earlier comments are all addressed! Thank you! Just noticed 1 last thing:

visited link should still get a color state color change, like the default state. The purple link on hover should turn dark blue.

joshblack

Seems great 👍 should we add tests too for this behavior, or should it be good enough as-is?

vpicone · 2019-09-04T01:56:34Z

packages/components/src/components/link/_link.scss

+      text-decoration: none;
+    }
+
+    &:visited {


Do we need to define this behavior a second time? since it's covered by the regular link class now? Having it match two rules means unintended specificity.

Good catch, thanks.

vpicone · 2019-09-04T01:56:43Z

packages/components/src/components/link/_link.scss

+    }
+
+    &:visited {
+      text-decoration: none;


Duplicate pseudo selector

Good catch, thanks.

tw15egan · 2019-09-04T19:31:54Z

packages/components/src/components/link/link.hbs

-<p class="{{@root.prefix}}--link--disabled">Disabled Link</p>
+<a href="#" class="{{@root.prefix}}--link{{#if inline}} {{@root.prefix}}--link--inline{{/if}}">Link</a>
+<a class="{{@root.prefix}}--link{{#if inline}} {{@root.prefix}}--link--inline{{/if}}" aria-disabled="true">Placeholder link</a>
+<p class="{{@root.prefix}}--link--disabled{{#if inline}} {{@root.prefix}}--link--inline{{/if}}">Disabled Link</p>


Is there a reason this is a p element here? In the React component, the element remains an a element, and has both bx--link and bx--link--disabled added.

It came from #1802, seems that it was for achieving styling requirements. @emyarod Do you have a details? I thought we had a discussion somewhere, but where it was completely slipped my mind (my apologies). My quick searches and following links from #1802 didn't reveal any, but I may be missing something.

I think it was to emphasize that simply adding the disabled link class would not be enough to actually disable an anchor. I think an anchor without an href could replace the p example here

I can't find the conversation but we've talked about the disabled links before. For a11y the demo should be:

<span role="link" aria-disabled="true" class="{{@root.prefix}}--link--disabled{{#if inline}} {{@root.prefix}}--link--inline{{/if}}">Disabled Link</span>

It's not a paragraph so it shouldn't be using a p tag. The role tells screen reader users that the intention is that it's a link and the aria-disabled informs them that it's disabled

tw15egan · 2019-09-04T19:32:21Z

packages/react/src/components/Link/Link.js

+  const classNames = classnames(`${prefix}--link`, className, {
+    [`${prefix}--link--disabled`]: disabled,
+    [`${prefix}--link--inline`]: inline,
+  });
  return (
    <a href={other.disabled ? null : href} className={classNames} {...other}>


This should just be disabled, since we now pull disabled out

tw15egan · 2019-09-04T19:34:10Z

I'm still seeing inconsistent styling between the Vanilla and React implementations. Vanilla looks fine, but it is using a p instead of an a.

In React, I am still setting styles applied when disabled is set.

Seems like these styles may need to be updated, like removing pointer-events: none (so that
not-allowed cursor shows in React

carbon/packages/components/src/components/link/_link.scss

Line 47 in 934fa04

&:not([href]) {

tw15egan · 2019-09-04T19:37:02Z

cc @shixiedesign, should the link color be $interactive-01 or $link-01? Website states $interactive-01. Should that be fixed on the website, or should the color be changed here?

Ref: #3902

shixiedesign · 2019-09-04T20:01:25Z

Ooh good catch. @tw15egan Website's wrong in this case! We created $link-01 for....links. I will make a PR to correct that.

asudoh · 2019-09-05T04:03:32Z

@tw15egan Good point on vanilla/React parity. I made a change to align React to vanilla for now (in addition to other.disabled vs. disabled change), but pointer-events approach may be valid (I recall I was personally inclined to that at some point). Will wait for feedback from @emyarod.

tw15egan · 2019-09-05T04:06:14Z

@asudoh pointer-events disables is fine with me, just want to make sure we have parity between React and Vanilla. I guess it is more of a design question.

Thanks for the quick updates!

emyarod · 2019-09-06T13:22:22Z

it's worth noting that the spec has undergone several major changes since #1802, and currently it seems that setting pointer-events is not needed

asudoh · 2019-09-06T23:33:51Z

Thanks @emyarod for the input. Currently pointer-events is used only at https://github.com/carbon-design-system/carbon/blob/v10.6.0/packages/components/src/components/link/_link.scss#L50. Do you think we should remove it, too, and if it's the case, does it mean we should no longer do "non-clickable unless href exists" feature?

I guess what we have as-is (adding this PR) sounds good, but just double-checking. Thanks!

emyarod · 2019-09-09T13:23:53Z

@asudoh I think it's fine to leave that one as is since I believe we want to keep it nonclickable unless href exists

asudoh · 2019-09-10T06:15:26Z

Resetting the review status given all comments have been addressed. Feel free to jump in if anybody has further thoughts - Thanks everybody for your input!

All review comments has been addressed.

shixiedesign · 2019-09-10T15:43:33Z

Hi @asudoh!

Looks like link lost the active color in React? On mouse down it should take on $text-01

asudoh · 2019-09-11T03:07:47Z

@shixiedesign Thank you for your review! I see it turns to #171717 ($text-01) when I mouse-down on the link. I could also see that when I forced the active state by selecting the <a> in DevTools, selecting :hov icon and selecting :active. That said, wondering what happens if you force active state.

shixiedesign

Hmm now the active state is working for me. Thanks! Great work. 🎉

asudoh · 2019-09-12T01:15:38Z

Thanks @shixiedesign for reviewing! 🙇

feat(link): introduce inline variant

281af80

Fixes carbon-design-system#3228.

asudoh added status: ready for review 👀 status: visual review 🔍 labels Aug 29, 2019

asudoh requested a review from shixiedesign August 29, 2019 09:18

emyarod approved these changes Aug 29, 2019

View reviewed changes

Merge branch 'master' into link-inline

fc7ddea

tw15egan previously requested changes Aug 29, 2019

View reviewed changes

Merge branch 'master' into link-inline

3507023

shixiedesign reviewed Aug 29, 2019

View reviewed changes

Merge branch 'master' into link-inline

9d6416c

vpicone suggested changes Aug 30, 2019

View reviewed changes

Akira Sudoh added 2 commits September 2, 2019 11:32

Merge branch 'master' into link-inline

bf2d358

fix(link): incorporate spec update, etc

06cb8c6

shixiedesign reviewed Sep 3, 2019

View reviewed changes

shixiedesign requested review from vpicone and tw15egan September 3, 2019 15:10

joshblack approved these changes Sep 3, 2019

View reviewed changes

Merge branch 'master' into link-inline

3e02b42

vpicone previously requested changes Sep 4, 2019

View reviewed changes

fix(Link): fix upon review comments

934fa04

tw15egan reviewed Sep 4, 2019

View reviewed changes

tw15egan and others added 3 commits September 4, 2019 14:03

Merge branch 'master' into link-inline

550dd2a

Merge branch 'master' into link-inline

d47f7be

fix(Link): parity with vanilla

86e9e5b

Merge branch 'master' into link-inline

d4f148d

shixiedesign self-requested a review September 10, 2019 15:43

shixiedesign approved these changes Sep 12, 2019

View reviewed changes

Merge branch 'master' into link-inline

5151e1e

shixiedesign and others added 2 commits September 11, 2019 20:21

Merge branch 'master' into link-inline

0455245

Merge branch 'master' into link-inline

a23b1dd

asudoh merged commit f167fd6 into carbon-design-system:master Sep 12, 2019

asudoh deleted the link-inline branch September 12, 2019 11:30

abbeyhrt mentioned this pull request Oct 16, 2019

chore: update packages carbon-design-system/gatsby-theme-carbon#471

Merged

feat(link): introduce inline variant #3859

feat(link): introduce inline variant #3859

Conversation

asudoh commented Aug 29, 2019

Changelog

Testing / Reviewing

netlify bot commented Aug 29, 2019 • edited Loading

netlify bot commented Aug 29, 2019 • edited Loading

netlify bot commented Aug 29, 2019 • edited Loading

emyarod left a comment

Choose a reason for hiding this comment

tw15egan left a comment • edited Loading

Choose a reason for hiding this comment

shixiedesign left a comment • edited Loading

Choose a reason for hiding this comment

Disabled link has hover state (both vanilla & react):

Focus state should not have underline

vpicone left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shixiedesign commented Aug 30, 2019 • edited Loading

asudoh commented Sep 2, 2019

shixiedesign left a comment

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

vpicone Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

asudoh Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tw15egan commented Sep 4, 2019

tw15egan commented Sep 4, 2019

shixiedesign commented Sep 4, 2019 • edited Loading

asudoh commented Sep 5, 2019

tw15egan commented Sep 5, 2019

emyarod commented Sep 6, 2019

asudoh commented Sep 6, 2019

emyarod commented Sep 9, 2019

asudoh commented Sep 10, 2019

shixiedesign commented Sep 10, 2019

asudoh commented Sep 11, 2019

shixiedesign left a comment

Choose a reason for hiding this comment

asudoh commented Sep 12, 2019

netlify bot commented Aug 29, 2019 •

edited

Loading

netlify bot commented Aug 29, 2019 •

edited

Loading

netlify bot commented Aug 29, 2019 •

edited

Loading

tw15egan left a comment •

edited

Loading

shixiedesign left a comment •

edited

Loading

vpicone left a comment •

edited

Loading

shixiedesign commented Aug 30, 2019 •

edited

Loading

vpicone Sep 4, 2019 •

edited

Loading

asudoh Sep 4, 2019 •

edited

Loading

shixiedesign commented Sep 4, 2019 •

edited

Loading