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

Add Repeatable #360

Merged
merged 11 commits into from
Nov 27, 2024
Merged

Add Repeatable #360

merged 11 commits into from
Nov 27, 2024

Conversation

dani-mp
Copy link
Contributor

@dani-mp dani-mp commented Nov 27, 2024

Resolves:
Alternative 1 for adding support to repeatable links. Creates a wrapper type similar to Group.

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.

src/types/value/types.ts Outdated Show resolved Hide resolved
* A list of repeatable fields.
*/
export type Repeatable<
Fields extends Array<AnyRepeatableField> = Array<AnyRepeatableField>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is right, because when AnyRepeatableField is composed of more than one field the array won't be homogeneous.

* A list of repeatable fields.
*/
export type Repeatable<
Fields extends Array<AnyRepeatableField> = Array<AnyRepeatableField>,
Copy link
Member

@lihbr lihbr Nov 27, 2024

Choose a reason for hiding this comment

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

Suggested change
Fields extends Array<AnyRepeatableField> = Array<AnyRepeatableField>,
Fields extends AnyRepeatableField,

/**
* Any field that can be repeated.
*/
export type AnyRepeatableField = LinkField
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export type AnyRepeatableField = LinkField
export type AnyRepeatableField = Repeatable<LinkField>

@@ -46,6 +52,7 @@ export type AnyRegularField =
| BooleanField
| GeoPointField
| IntegrationField
| Repeatable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Repeatable
| AnyRepeatableField

* A repeatable link field.
*/

export type RepeatableLinkField = Repeatable<LinkField>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lihbr @angeloashmore i believe here we need to add to RepeatableLinkField the same type params of the LinkField, and forward them, right?

btw, i still feel that we don't need this extra type declaration, but let me know if you still want to keep it

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we don't need this. Repeatable<LinkField> is self-descriptive and more flexible.

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.

Requested changes based on the call we had yesterday. Let me know if things changed since then, but I prefer using a utility type (Repeatable) over field-specific types (RepeatableLinkField).

* A repeatable link field.
*/

export type RepeatableLinkField = Repeatable<LinkField>
Copy link
Member

Choose a reason for hiding this comment

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

I agree, we don't need this. Repeatable<LinkField> is self-descriptive and more flexible.

* @returns `true` if `repeatable` contains at least one item, `false`
* otherwise.
*/

Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Small nit: no space between TSDoc and the implementation.

src/helpers/isFilled.ts Outdated Show resolved Hide resolved
@@ -35,6 +35,7 @@ export type AnyRegularField =
| ImageField
| ContentRelationshipField
| LinkField
| RepeatableLinkField
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| RepeatableLinkField
| Repeatable<LinkField>

dani-mp and others added 4 commits November 27, 2024 22:00
Co-authored-by: Angelo Ashmore <angeloashmore@users.noreply.github.com>
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.

Perfect!

@dani-mp dani-mp changed the title Add Repeatable wrapper Add Repeatable Nov 27, 2024
@dani-mp dani-mp merged commit 00178e5 into lg/repeatable-links Nov 27, 2024
12 checks passed
@dani-mp dani-mp deleted the dani/repeatable-links-alt1 branch November 27, 2024 21:55
dani-mp added a commit that referenced this pull request Dec 5, 2024
* chore: bump types-internal dep

* feat: support repeatable links in models

* Bump types internal

* Remove repeat from content rel. and link to media

* Add Repeatable (#360)

* Add Repetable to index

* chore(release): 7.13.0-alpha.0

* Use stable types internal

---------

Co-authored-by: Daniel Martín <danimartinprieto@gmail.com>
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