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

feat: Create Snackbar When Copying Schedules #811

Merged
merged 12 commits into from
Dec 4, 2023
Merged

Conversation

KevinWu098
Copy link
Member

Summary

  1. Open snackbar when copying (appending) schedules; happens on success and on failure

Snackbar opens on success

Test Plan

  1. Check that the snackbar opens appropriately on success and on failure

Issues

Closes #

Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

LGTM!

@KevinWu098 KevinWu098 requested a review from ap0nia December 2, 2023 18:25
@KevinWu098
Copy link
Member Author

KevinWu098 commented Dec 2, 2023

I'm not sure if my type definition for the CallbackOptions is proper, so just lmk if there's a better way to do them @ap0nia

export interface CallbackOptions {
    onSuccess: (name?: string) => unknown;
    onError: (name?: string) => unknown;
}

@KevinWu098 KevinWu098 removed the request for review from Douglas-Hong December 2, 2023 18:30
Copy link
Collaborator

@ap0nia ap0nia left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the strategy 🙏

I think the original function signature should be completely preservable, i.e. it should accept just the index and possibly options. The options will be callback functions that will be called with the index after a successful/unsuccessful attempt to copy the schedule.

The callback functions can figure out the schedule name by indexing the array of schedule names with the index they were called with.

apps/antalmanac/src/actions/AppStoreActions.ts Outdated Show resolved Hide resolved
apps/antalmanac/src/actions/AppStoreActions.ts Outdated Show resolved Hide resolved
apps/antalmanac/src/actions/AppStoreActions.ts Outdated Show resolved Hide resolved
@KevinWu098 KevinWu098 requested a review from ap0nia December 4, 2023 01:33
@KevinWu098 KevinWu098 requested a review from ap0nia December 4, 2023 01:57
@ap0nia
Copy link
Collaborator

ap0nia commented Dec 4, 2023

I kinda wanna write tests for it, but I don't wanna write tests for it lol.

What do you wanna do Kevin? If you don't wanna test, then I'll just approve and merge 🤓

@KevinWu098
Copy link
Member Author

I kinda wanna write tests for it, but I don't wanna write tests for it lol.

What do you wanna do Kevin? If you don't wanna test, then I'll just approve and merge 🤓

If it breaks it breaks, and somebody gets extra contribution history 😋

@KevinWu098
Copy link
Member Author

TLDR let's merge 😅, do you need me to do a noop?

Copy link
Collaborator

@ap0nia ap0nia left a comment

Choose a reason for hiding this comment

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

Good work refactoring. This was more involved than it could've been otherwise, but I have to slow down Kevin from moving too fast 💯

@ap0nia
Copy link
Collaborator

ap0nia commented Dec 4, 2023

Yeah I think I need an extra commit from someone aside from me lol. Unless you're able to merge it yourself.

@ap0nia ap0nia self-requested a review December 4, 2023 02:08
@ap0nia
Copy link
Collaborator

ap0nia commented Dec 4, 2023

image
lol it's not working

@EricPedley EricPedley merged commit 75fd50f into main Dec 4, 2023
5 of 6 checks passed
@EricPedley EricPedley deleted the snackbar-copy branch December 4, 2023 02:12
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.

4 participants