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

[Link] Improve docblocks and readme for links #2153

Merged
merged 2 commits into from
Sep 18, 2019
Merged

[Link] Improve docblocks and readme for links #2153

merged 2 commits into from
Sep 18, 2019

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Sep 17, 2019

WHY are these changes introduced?

People are unclear if the external prop on links should be used for sites external to shopify, or anything that opens in a new tab.

You should use it for anything that opens in a new tab.

WHAT is this pull request doing?

Make it clear external is to be used for an page that opens in a new tab
Make wording and ordering consistent between Link and UnstyledLink

Clarify readme to make it clear the important thing is "you're going anywhere in a new tab", not "you're going outside your current domain"

Make it clear external is to be used for an page that opens in a new tab
Make wording and ordering consistent between Link and UnstyledLink
@BPScott BPScott added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Sep 17, 2019
kaelig
kaelig previously requested changes Sep 17, 2019
Copy link
Contributor

@kaelig kaelig 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 this PR's scope has gone beyond docblocks.

Can you update the title to reflect the new scope, and prefix it with the component's name?

Also, I'd say it's worth having a CHANGELOG entry for this.

@BPScott BPScott changed the title Improve docblocks for links [Link] Improve docblocks and readme for links Sep 17, 2019
Copy link
Contributor

@selenehinkley selenehinkley left a comment

Choose a reason for hiding this comment

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

The copy looks good to me! Thanks for making this update, Ben!

@BPScott BPScott dismissed kaelig’s stale review September 18, 2019 16:24

concerns addressed

@BPScott BPScott merged commit 00808ec into master Sep 18, 2019
@BPScott BPScott deleted the link-details branch September 18, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖Skip Changelog Causes CI to ignore changelog update check.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants