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 variant #364

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

Link variant #364

wants to merge 8 commits into from

Conversation

dani-mp
Copy link
Contributor

@dani-mp dani-mp commented Dec 11, 2024

Resolves:
Part of https://linear.app/prismic/issue/DT-2469/aauser-when-i-create-a-link-field-and-allow-variants-i-can-see-an.

Description

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.

@dani-mp dani-mp requested a review from xrutayisire December 12, 2024 11:47
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 so far! I left a few questions and suggestions.

src/types/value/link.ts Show resolved Hide resolved
@@ -45,4 +45,5 @@ export interface FilledContentRelationshipField<
isBroken?: boolean
data?: DataInterface
text?: string
variant?: string
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ #issue: From this Slack message, it seems we are only supporting variant on link fields, not content relationship and link to media fields.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need to add the variant property in src/types/value/link.ts as an intersection. Something like this pseudo-code:

type LinkField = (WebLinkField | LinkToMediaField | ContentRelationshipField) & {
  variant?: string
}

That keeps variant out of link to media and content relationship fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first message: i think what we want is to not add this option to the link to media or content relationship concrete fields, but you can still create a generic link without the select and then in page builder select one of the types

second message: i understood from a conversation with @xrutayisire the other day that one might want to use a concrete version of the field and it should already contain all properties, as they're all exported individually

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, at first glance, the proposal appears promising. I like it. I believed we always wanted to have the properties defined in their respective files. 🧐

@@ -25,4 +25,5 @@ export interface FilledLinkToMediaField {
height?: string | null
width?: string | null
text?: string
variant?: string
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ #issue: From this Slack message, it seems we are only supporting variant on link fields, not content relationship and link to media fields.

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