Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-hyperlink] Switched themeable icons provided by CSS to imported terra-icon component. #2666

Merged
merged 15 commits into from
Nov 8, 2019
Merged

[terra-hyperlink] Switched themeable icons provided by CSS to imported terra-icon component. #2666

merged 15 commits into from
Nov 8, 2019

Conversation

nramamurth
Copy link
Contributor

@nramamurth nramamurth commented Sep 27, 2019 •
edited
Loading

Summary

Closes #1832.

Deployment Links:

@nramamurth nramamurth self-assigned this Sep 27, 2019
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 September 27, 2019 10:36 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 September 30, 2019 05:02 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 September 30, 2019 05:43 Inactive
@neilpfeiffer
Copy link
Member

neilpfeiffer commented Oct 2, 2019 •
edited
Loading

Removing Major Version Bump label since this will have no consumer-facing API changes and internal implementation updates only. It does affect themes, so will require all theme-work be done as well, prior to closing PR.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 October 3, 2019 07:28 Inactive
@PaulSolDev PaulSolDev marked this pull request as ready for review October 8, 2019 15:47
@neilpfeiffer neilpfeiffer mentioned this pull request Oct 14, 2019
Closed
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 October 15, 2019 04:34 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-2666 October 15, 2019 05:30 Inactive
@nramamurth nramamurth changed the title [WIP] [terra-hyperlink] Switched themeable icons provided by CSS to imported terra-icon component. [terra-hyperlink] Switched themeable icons provided by CSS to imported terra-icon component. Oct 15, 2019
StephenEsser
StephenEsser reviewed Oct 25, 2019
View reviewed changes
packages/terra-hyperlink/src/Hyperlink.module.scss
}
}

.document.is-disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the disabled states appropriately handled with introducing terra-icons and inheriting certain dithering and opacity? Previously the disabled states had different icons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd think that the current change is expected, going by @neilpfeiffer's comment on the issue.

This will allow the icons to be able to take the current-color of the text for all states (hover, active, visited, inactive, etc.)

Neil, I am sorry if I have misquoted but could you please confirm it?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is what he's saying. And that's working.

@jeremyfuksa jeremyfuksa requested review from jeremyfuksa and removed request for bjankord November 7, 2019 18:49
jeremyfuksa
jeremyfuksa approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@jeremyfuksa jeremyfuksa left a comment

Choose a reason for hiding this comment

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

+1. Ready for release.

@tbiethman tbiethman merged commit 1061a1a into master Nov 8, 2019
@tbiethman tbiethman deleted the css-to-jsx branch November 8, 2019 03:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeremyfuksa jeremyfuksa jeremyfuksa approved these changes

@tbiethman tbiethman tbiethman approved these changes

@jmsv6d jmsv6d jmsv6d approved these changes

@StephenEsser StephenEsser StephenEsser approved these changes

@mjhenkes mjhenkes Awaiting requested review from mjhenkes

@ryanthemanuel ryanthemanuel Awaiting requested review from ryanthemanuel

@avinashg1994 avinashg1994 Awaiting requested review from avinashg1994

@lokesh-0813 lokesh-0813 Awaiting requested review from lokesh-0813

@pranav300 pranav300 Awaiting requested review from pranav300

@saket2403 saket2403 Awaiting requested review from saket2403

@ShettyAkarsh ShettyAkarsh Awaiting requested review from ShettyAkarsh

@supreethmr supreethmr Awaiting requested review from supreethmr

@neilpfeiffer neilpfeiffer Awaiting requested review from neilpfeiffer

@dkasper-was-taken dkasper-was-taken Awaiting requested review from dkasper-was-taken

Assignees

@StephenEsser StephenEsser

@nramamurth nramamurth

Labels
📦 terra-hyperlink
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Terra Hyperlink update icons from css to jsx
8 participants
@nramamurth @neilpfeiffer @jeremyfuksa @tbiethman @jmsv6d @ryanthemanuel @StephenEsser @bjankord