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 missing params for new tracker api #147

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

@eperedo eperedo requested a review from tokland May 16, 2024 20:15
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

Comments in-line.

(Note: Having the clickup issue in the description of the PR is very handy to add time for the review, specially for PRs for common libraries)

package.json Outdated Show resolved Hide resolved
src/api/trackerTrackedEntities.ts Outdated Show resolved Hide resolved
@eperedo eperedo requested a review from tokland June 19, 2024 13:46
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

Some minor comments:

| "updatedAt";
};

export type TrackedAttributesFields = { type: "id"; id: Id };
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, TS uses singular names for this kind of types.

| "updatedAt";
};

export type TrackedAttributesFields = { type: "id"; id: Id };
Copy link
Contributor

@tokland tokland Aug 6, 2024

Choose a reason for hiding this comment

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

The name of type makes it clear within the module, but the plain object is created (example: { type: "id", id: "wMhqqPLb7pP", direction: "desc" }), we don't know what is the model of that id. Maybe type: "trackedEntityAttributeId"? (+ update README)

@eperedo eperedo requested a review from tokland August 7, 2024 12:42
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

[code only review]

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