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.

Sign up for GitHub

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

Jump to bottom

Bugfix/2361 euilink disabled #2423

Merged
merged 7 commits into from
Oct 21, 2019
Merged

Bugfix/2361 euilink disabled #2423

merged 7 commits into from
Oct 21, 2019

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Oct 12, 2019 •
edited
Loading

Summary

Fixes #2361

Made the EuiLink component look non-interactive when it is given the disabled prop

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@elasticcla
Copy link

Hi @ThomThomson, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@ThomThomson ThomThomson reopened this Oct 12, 2019
@ThomThomson ThomThomson marked this pull request as ready for review October 12, 2019 01:45
@thompsongl
Copy link
Contributor

jenkins test this

thompsongl
thompsongl suggested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks!

Looks like it satisfies the resolution @cchaos describes in the ticket, but I'll let @snide or another designer verify.

CHANGELOG.md Outdated Show resolved Hide resolved
src/components/link/_link.scss Outdated Show resolved Hide resolved
thompsongl
thompsongl suggested changes Oct 14, 2019
View reviewed changes
src/components/link/link.tsx Outdated Show resolved Hide resolved
cchaos
cchaos suggested changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ThomThomson !! I have a couple of comments.

src/components/link/_link.scss Outdated Show resolved Hide resolved
src/components/link/link.tsx Outdated Show resolved Hide resolved
src/components/link/_link.scss Outdated Show resolved Hide resolved
src-docs/src/views/link/link_example.js Outdated Show resolved Hide resolved
src-docs/src/views/link/link_example.js Outdated Show resolved Hide resolved
src-docs/src/views/link/link_example.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cchaos
cchaos reviewed Oct 17, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning those up. One last thing is that we will need a test created for this new prop. You should be able to copy/paste and follow the same patterns as the other props in the test file. Then you'll need to run yarn run test-unit -u to update the snapshots.

@cchaos
Copy link
Contributor

cchaos commented Oct 18, 2019

Thanks @ThomThomson. It looks like there's now a conflict from another PR that touched the EuiLink code. Can you fix the conflicts? If you run into trouble, I can try to push you a PR.

cchaos
cchaos approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Oh wait nevermind, sorry about that. The conflict was only on my end when I just tried to pull your changes directly without resetting with master.

The new markup looks good to me!

@thompsongl
Copy link
Contributor

jenkins test this

thompsongl
thompsongl suggested changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Looks like a simple Prettier lint error.

Committing the results of yarn lint-fix will get it resolved and this will be ready to merge

@thompsongl
Copy link
Contributor

jenkins test this

@cchaos cchaos merged commit d40b288 into elastic:master Oct 21, 2019
@thompsongl thompsongl mentioned this pull request Oct 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@thompsongl thompsongl thompsongl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disabled EuiLink styled like regular EuiLink
5 participants
@ThomThomson @elasticmachine @elasticcla @thompsongl @cchaos