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

feat: support link text #23

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: support link text #23

wants to merge 7 commits into from

Conversation

xrutayisire
Copy link
Collaborator

Resolves: DT-2272

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

  • Support link text

Checklist:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

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

Like the other PRs, this looks good! I left a few suggestions, none of which are blocking.

I'm going to mark the PR as "Request changes" because we should publish @prismicio/client first and update the version here before merging. Let me know if you'd like to handle that a different way though.

src/PrismicLink.svelte Outdated Show resolved Hide resolved
src/PrismicLink.svelte Outdated Show resolved Hide resolved
test/PrismicLink.test.ts Outdated Show resolved Hide resolved
test/PrismicLink.test.ts Outdated Show resolved Hide resolved
test/PrismicLink.test.ts Outdated Show resolved Hide resolved
test/PrismicLink.test.ts Outdated Show resolved Hide resolved
test/PrismicLinkTestWrapper.svelte Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.39%. Comparing base (bafcd24) to head (e943cff).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/PrismicLink.svelte 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   67.83%   67.39%   -0.44%     
==========================================
  Files          18       18              
  Lines         914      914              
  Branches        1        5       +4     
==========================================
- Hits          620      616       -4     
- Misses        294      298       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

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

Just need to remember to update the package versions before merging and publishing. 🙂

Re: prismicio/prismic-react#205 (comment), fallback <slot> values mostly already work the same way as "children" in props" ? children : text. Svelte essentially runs .trim() on the children, which has slightly different behavior:

<!-- These examples render `field.text` -->
<PrismicLink field={field}></PrismicLink>
<PrismicLink field={field}> </PrismicLink>
<PrismicLink field={field}>  </PrismicLink>

<!-- These examples render the children as literal values -->
<PrismicLink field={field}>{undefined}</PrismicLink>
<PrismicLink field={field}>{null}</PrismicLink>
<PrismicLink field={field}>{""}</PrismicLink>
<PrismicLink field={field}>{0}</PrismicLink>

Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

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

Just a couple of small things. 🙂

src/PrismicLink.svelte Outdated Show resolved Hide resolved
src/PrismicLink.svelte Outdated Show resolved Hide resolved
Copy link
Member

@angeloashmore angeloashmore left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

3 participants