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

Feature/calendar-kiron #8

Draft
wants to merge 59 commits into
base: main
Choose a base branch
from
Draft

Feature/calendar-kiron #8

wants to merge 59 commits into from

Conversation

K02D
Copy link
Collaborator

@K02D K02D commented Apr 24, 2022

My attempt at letting users edit their previous times

RosaGao and others added 30 commits March 27, 2022 11:14
@K02D K02D marked this pull request as ready for review April 24, 2022 22:52
@yesterd3v
Copy link
Collaborator

Yeah that's a good idea. I'll make this PR ready for review and once James/Sebastian approves we can merge it

Make sure to add us as reviewers!

@K02D K02D requested review from yesterd3v and removed request for yesterd3v April 26, 2022 00:12
@K02D K02D requested review from yesterd3v and removed request for yesterd3v April 26, 2022 00:13
@K02D
Copy link
Collaborator Author

K02D commented Apr 26, 2022

Oh okay. Sorry about all the notifications! Are you added as a reviewer now?

@RosaGao
Copy link
Collaborator

RosaGao commented Apr 26, 2022

image

The share button seems to be buggy. I'm not sure if it's only on my computer but the avails didn't show when members are selected and the above error was given.

@K02D
Copy link
Collaborator Author

K02D commented Apr 26, 2022

the avails didn't show when members are selected and the above error was given

Is the app not working like it was in the demo? If not I'll look into the code and see what's going on

@RosaGao
Copy link
Collaborator

RosaGao commented Apr 26, 2022

The concurrently rendering thing is fixed. We would need to wrap setState inside useEffect.

@RosaGao
Copy link
Collaborator

RosaGao commented Apr 26, 2022

Some other issues that we need to consider:

Changes made inside confirm is not save to the app store, so the next time we select that single member wanting to make edits, only the previous selecitons (ones before hitting confirm) will appear in green. But since the changes are post to the api, the changes/edits made through that confirm will show.

If we refresh the page (which is the same as reentering through the shared link), all selections seem to be lost.

@RosaGao
Copy link
Collaborator

RosaGao commented Apr 26, 2022

The app works well like what's in the demo (besides perhaps the confirm part) if we stay on the same page. Also I am a little confused by the purpose changedAlready? If it is only needed inside Calendar it might be changed to a local state rather than a global one

@K02D
Copy link
Collaborator Author

K02D commented Apr 26, 2022

If it is only needed inside Calendar it might be changed to a local state rather than a global one

That's what I did initially but if we use useState() inside Calendar then we'd initialize changedAlready every time Calendar gets rendered, which wouldn't work

The purpose of ChangedAlready is to prevent the infinite re-rending caused by forcibly changing the selection (green cells)

@K02D
Copy link
Collaborator Author

K02D commented Apr 26, 2022

You're right about the Confirm button not working. Hmmm it looks like it's not even working like it was in the demo. I made 1 commit after the demo and that might've broken something. I'll look into it later in the week. Thanks for letting me know

@JJamesWWang
Copy link
Member

Since it seems there's an issue, let me know when this is ready for review again

@K02D
Copy link
Collaborator Author

K02D commented Apr 30, 2022

Okay the reason why the app isn't working now but worked during the demo is because my function for converting timeSlots to Date is incorrect (it happened to work for the date of the demo)

@K02D
Copy link
Collaborator Author

K02D commented Apr 30, 2022

@RosaGao I've fixed the problems with Add and Confirm (my conversion function was incorrect). You could have a look and see if everything's working like in the demo!

@RosaGao
Copy link
Collaborator

RosaGao commented May 1, 2022

Sounds good!!

@K02D K02D marked this pull request as draft December 29, 2022 13:06
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