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

chore: Update types to be consistent with documentation #244

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

thales-fukuda
Copy link
Contributor

@thales-fukuda
Copy link
Contributor Author

@pawelgrimm, I don't think I have permission to assign you as a reviewer. I hope you don't mind the direct tag, thank you!

@pawelgrimm pawelgrimm self-requested a review October 23, 2023 21:30
@pawelgrimm
Copy link
Contributor

Thanks, @thales-fukuda! I'll try to take a look at this tomorrow or on Thursday.

Copy link
Contributor

@pawelgrimm pawelgrimm left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! There are a few things I'd like to change slightly, but I'll go ahead and do that myself.

@@ -84,7 +85,7 @@ export const Project = Record({
isInboxProject: Boolean,
isTeamInbox: Boolean,
order: Int,
viewStyle: String,
viewStyle: Union(Literal('list'), Literal('board')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically the API could return something other than "list" or "board" here.

Let's keep this as String, but export a new type here for ProjectViewStyle to get intellisense for requests, like adding or updating projects.

Suggested change
viewStyle: Union(Literal('list'), Literal('board')),
viewStyle: String,
export type ProjectViewStyle = 'list' | 'board'

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Addressed in 16e51a1


export type ProjectViewStyle = 'list' | 'board'
export type ProjectViewStyle = Project['viewStyle']
Copy link
Contributor

@pawelgrimm pawelgrimm Oct 26, 2023

Choose a reason for hiding this comment

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

Let's just reference Project['viewStyle'] instead of creating another type for it here.

Suggested change
export type ProjectViewStyle = Project['viewStyle']

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Addressed in 16e51a1

Copy link
Contributor

Choose a reason for hiding this comment

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

Ope, I guess we can't do that because it breaks validators.validateProject. Let's reference the ProjectViewStyle type here.

Comment on lines 15 to 18
} & RequireOneOrNone<{
dueString?: string
dueDate?: string
dueDatetime?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

You can specify a dueString and a dueDate/dueDatetime. This is because the dueString could indicate the recurrence (e.g. "every day at 9am"), whereas the dueDatetime could indicate the due time of the current instance of the task (e.g. "2023-10-26T12:00:00").

Suggested change
} & RequireOneOrNone<{
dueString?: string
dueDate?: string
dueDatetime?: string
dueString?: string
} & RequireOneOrNone<{
dueDate?: string
dueDatetime?: string

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Addressed in 8b1c471

Comment on lines 48 to 49
} & RequireOneOrNone<{
dueString?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

♻️ Same situation as my previous comment

Suggested change
} & RequireOneOrNone<{
dueString?: string
dueString?: string
} & RequireOneOrNone<{

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Addressed in 8b1c471

} & RequireExactlyOne<{
taskId?: string
projectId?: string
}>

export type AddTaskCommentArgs = AddCommentArgs & {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid of AddTaskCommentArgs and Add ProjectCommentArgs now 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ Addressed in 97adeb1

@pawelgrimm pawelgrimm merged commit bde976d into Doist:main Oct 26, 2023
1 check passed
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.

Types that are missing or could be improved
2 participants