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 #205

Merged
merged 11 commits into from
Sep 27, 2024
Merged

feat: support link text #205

merged 11 commits into from
Sep 27, 2024

Conversation

xrutayisire
Copy link
Contributor

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
  • Update example with new usage

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

github-actions bot commented Aug 28, 2024

size-limit report 📦

Path Size
./dist/index.cjs 8.15 KB (+1.54% 🔺)
./dist/index.js 6.1 KB (+0.8% 🔺)
./dist/react-server.cjs 7.36 KB (+1.94% 🔺)
./dist/react-server.js 5.64 KB (+1% 🔺)

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.

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/react-server/PrismicLink.tsx Outdated Show resolved Hide resolved
src/react-server/PrismicLink.tsx Outdated Show resolved Hide resolved
src/react-server/PrismicLink.tsx Outdated Show resolved Hide resolved
test/PrismicLink.test.tsx Outdated Show resolved Hide resolved
test/PrismicLink.test.tsx Outdated Show resolved Hide resolved
test/PrismicLink.test.tsx Outdated Show resolved Hide resolved
test/PrismicLink.test.tsx Outdated Show resolved Hide resolved
test/react-server/PrismicRichText.test.tsx Show resolved Hide resolved
@xrutayisire
Copy link
Contributor Author

On the PR status, we could go with approved or not anyway. We have the release process that will take in place after, where we need to update all of our work with latest packages. If it's not approved and with the time difference, it may mean we release without clear green approval on your side.
But if you comment that it is good except the packages, I guess we would be good then and understand we can release 🙂

@angeloashmore
Copy link
Member

@xrutayisire Could we discuss and conclude #205 (comment) before I approve this and the other SDK PRs? You're right about handling the version numbers after the approval.

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.

Looks good!

@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 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.84%. Comparing base (d7f196f) to head (a440cff).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/react-server/PrismicLink.tsx 95.65% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   99.68%   99.84%   +0.15%     
==========================================
  Files          20       20              
  Lines        2569     2585      +16     
  Branches       97       99       +2     
==========================================
+ Hits         2561     2581      +20     
+ Misses          8        4       -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.

Looks good!

Comment on lines +138 to +140
("text" in field
? Object.keys(field).length > 2
: Object.keys(field).length > 1) &&
Copy link
Member

@angeloashmore angeloashmore Sep 13, 2024

Choose a reason for hiding this comment

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

🎉 #nice: The dev message code is still a little weird, but this is a nice and simple update to make your new work compatible. 😄

@dani-mp dani-mp merged commit c41ffb5 into master Sep 27, 2024
1 check failed
@dani-mp dani-mp deleted the xru/support-link-text branch September 27, 2024 10:44
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.

5 participants