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

Added logic to add bookmark #118

Merged
merged 2 commits into from
Jul 14, 2024
Merged

Added logic to add bookmark #118

merged 2 commits into from
Jul 14, 2024

Conversation

shin-usu
Copy link
Contributor

@shin-usu shin-usu commented Jul 11, 2024

Overview (Required)

  • Added logic to add bookmark in TimetableDetailView
  • Added navigation logic to timetable detail from timetable list
  • Modified test code

Movie (Optional)

Before After
2024-07-12.1.21.12.mov

@shin-usu shin-usu force-pushed the feature/add_bookmark branch 2 times, most recently from 8b66073 to e2738cc Compare July 11, 2024 16:44
Comment on lines +7 to +9
streamTimetable: unimplemented("TimetableClient.streamTimetable"),
streamTimetableItemWithFavorite: unimplemented("TimetableClient.streamTimetableItemWithFavorite"),
toggleBookmark: unimplemented("TimetableClient.toggleBookmark")
Copy link
Contributor Author

@shin-usu shin-usu Jul 11, 2024

Choose a reason for hiding this comment

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

Changed to use unimplemented as recommended in swift-dependencies libraries.
I changed to mock functions that are needed in unit tests.
https://pointfreeco.github.io/swift-dependencies/main/documentation/dependencies/livepreviewtest/#Test-value

@shin-usu shin-usu force-pushed the feature/add_bookmark branch 2 times, most recently from b699899 to a871d00 Compare July 11, 2024 16:52
@shin-usu shin-usu force-pushed the feature/add_bookmark branch from a871d00 to d3acfea Compare July 11, 2024 16:57
@shin-usu shin-usu marked this pull request as ready for review July 11, 2024 16:57
Base automatically changed from feature/toast_ios to main July 11, 2024 17:55
Copy link
Contributor

@MrSmart00 MrSmart00 left a comment

Choose a reason for hiding this comment

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

THX!!!
I left a comment. please check it👍

Comment on lines 45 to 51
do {
await send(.bookmarkResponse(.success(
try await timetableClient.toggleBookmark(id: TimetableItemId(value: ""))
)))
} catch {
await send(.bookmarkResponse(.failure(error)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
do {
await send(.bookmarkResponse(.success(
try await timetableClient.toggleBookmark(id: TimetableItemId(value: ""))
)))
} catch {
await send(.bookmarkResponse(.failure(error)))
}
await send(.bookmarkResponse(Result {
try await timetableClient.toggleBookmark(id: TimetableItemId(value: ""))
}))

I guess you could change these like that🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's smarter that way. Thx!

@shin-usu shin-usu requested a review from MrSmart00 July 12, 2024 02:35
@shin-usu
Copy link
Contributor Author

@MrSmart00
Thank you for your comment.
Please check again🙏🏻

Copy link
Contributor

@MrSmart00 MrSmart00 left a comment

Choose a reason for hiding this comment

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

LGTM🎉

Copy link
Member

@ry-itto ry-itto left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼 Thanks!

@shin-usu shin-usu merged commit b9b63c4 into main Jul 14, 2024
@shin-usu shin-usu deleted the feature/add_bookmark branch July 14, 2024 17:03
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