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

fix: fix link types value + model #366

Closed
wants to merge 1 commit into from

Conversation

xrutayisire
Copy link
Contributor

Description

This PR objective is to clean up and fix the link types (model + value).

This PR initially assumes that we will support allowText and variants for Link To Media and Generic Link.
We are waiting for a final product decision and, of course, this will impact the code if we decide to drop support, as it's the case for Content Relationship.

Model

  • allowText should not be defined on Content Relationship
  • variants should be defined for Link To Media.

Value

  • Ensure all Filled links have text and variants as they are re-used everywhere to define what a link even coming from a generic link can be.
  • Ensure EmptyLinkField doesn't by default have text and variant as it's used in ContentRelationshipField
  • Omit text and variant manually when types are used for Rich Text

Checklist

  • A comprehensive Linear ticket, providing sufficient context and details to facilitate the review of the PR, is linked to the PR.
  • If my changes require tests, I added them.
  • If my changes affect backward compatibility, it has been discussed.
  • If my changes require an update to the CONTRIBUTING.md guide, I updated it.

Preview

How to QA 1

Footnotes

  1. Please use these labels when submitting a review:
    ❓ #ask: Ask a question.
    💡 #idea: Suggest an idea.
    ⚠️ #issue: Strongly suggest a change.
    🎉 #nice: Share a compliment.

Copy link
Contributor

@dani-mp dani-mp left a comment

Choose a reason for hiding this comment

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

left a question and a suggestion

src/types/value/contentRelationship.ts Show resolved Hide resolved
*
* @typeParam Link - Link field.
*/
export type WithLinkAdditionalData<Link> = Link & {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about

Suggested change
export type WithLinkAdditionalData<Link> = Link & {
export type WithAdditionalLinkAttributes<Link> = Link & {

another option: WithAdditionalLinkProps

@dani-mp dani-mp deleted the xru/fix-link-types branch December 17, 2024 18:39
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.

2 participants