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.

Sign up for GitHub

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

Jump to bottom

bug/252-Fix checkbox not updating by abstracting state away from component renders closes #252 #254

Closed
wants to merge 5 commits into from
Closed

bug/252-Fix checkbox not updating by abstracting state away from component renders closes #252 #254

wants to merge 5 commits into from

Conversation

preetpatel
Copy link
Contributor

@preetpatel preetpatel commented Apr 17, 2020 •
edited by EricPedrido
Loading

  • The pull request is complete according to the following criteria:
    • Acceptance criteria have been met
    • The documentation is kept up-to-date
    • Comprehensive tests (if applicable) have been generated and all pass.
    • The pull request describes the changes that have been made, and enough information is present in the description for any developer to understand what has changed
    • Commits have been squashed (or will be on merge).
    • The branch name is descriptive and follows the pull request title format : {issue/bug...}/(Issue Number) - Name of issue. E.g bug/30-Fix-Project
    • The pull request title is of the following format : {issue/bug...}/(Issue Number) - Name of issue. E.g bug/30-Fix-Project
    • The description uses github syntax to link to the issue. E,g Resolves se701g2/Doto#{Number}
    • At least two reviewers assigned. One of which must be the assigner of the issue.
    • If there are merge conflicts, run git rebase as opposed to git merge with master.

@preetpatel preetpatel linked an issue Apr 17, 2020 that may be closed by this pull request
Checkbox in to-do list items doesn't update correctly #252
Open
EricPedrido
EricPedrido previously requested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@EricPedrido EricPedrido left a comment

Choose a reason for hiding this comment

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

It crosses out the task on the checklist and the task is marked as complete which is good. However, the actual checkbox doesn't get ticked. If there's no way to get the checkbox to be ticked through code then I guess it's alright since the actual task is marked as complete, but it's just a cosmetic problem.

@preetpatel
Copy link
Contributor Author

It crosses out the task on the checklist and the task is marked as complete which is good. However, the actual checkbox doesn't get ticked. If there's no way to get the checkbox to be ticked through code then I guess it's alright since the actual task is marked as complete, but it's just a cosmetic problem.

It did the tick for me when i ran it.. thats weird

@harmanlamba harmanlamba self-requested a review April 17, 2020 10:12
harmanlamba
harmanlamba approved these changes Apr 17, 2020
View reviewed changes
Copy link

@harmanlamba harmanlamba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @preetpatel ,

Thanks for working on this bug. I think that I might have found a new bug while checking this code out locally. When first accessing Doto I get the following:

  • After the authentication the following exception is thrown, "Unhandled Rejection (Type Error): Cannot read property 'themePreference". Although, there is a button to cross it out and proceed.
  • This then loads the home page, when trying to create a new task the following is thrown: "Cannot read property 'forEach"

I have attached screenshots below for reference (To document with the bug).
image
image

NOTE:
After doing a refresh everything works as supposed to and I do not get the issue @EricPedrido mentioned. Thanks for fix @preetpatel 👍. Thus, since everything does seem to work I have gone ahead and approved the PR and will make a bug report for what I encountered, and hopefully, we can triangulate it soon.

@preetpatel preetpatel requested a review from EricPedrido April 18, 2020 03:01
@preetpatel
Copy link
Contributor Author

preetpatel commented Apr 18, 2020 •
edited
Loading

Hi @preetpatel ,

Thanks for working on this bug. I think that I might have found a new bug while checking this code out locally. When first accessing Doto I get the following:

  • After the authentication the following exception is thrown, "Unhandled Rejection (Type Error): Cannot read property 'themePreference". Although, there is a button to cross it out and proceed.
  • This then loads the home page, when trying to create a new task the following is thrown: "Cannot read property 'forEach"

I have attached screenshots below for reference (To document with the bug).
image
image

NOTE:
After doing a refresh everything works as supposed to and I do not get the issue @EricPedrido mentioned. Thanks for fix @preetpatel 👍. Thus, since everything does seem to work I have gone ahead and approved the PR and will make a bug report for what I encountered, and hopefully, we can triangulate it soon.

Cheers thanks @harmanlamba
I think the issue you're having is due to not properly going through the auth flow for google

@EricPedrido
Copy link
Contributor

EricPedrido commented Apr 18, 2020 •
edited
Loading

I do not get the issue @EricPedrido mentioned

Alright, well I assume it's a problem on my end then, I'll remove my change request but I won't be approving it just in case someone else gets this issue too. Nice work though, everything else looks good.

@EricPedrido EricPedrido dismissed their stale review April 18, 2020 10:34

Works for everyone else

@HarrisonLeach1 HarrisonLeach1 changed the title bug/252-Fix checkbox not updating by abstracting state away from component renders closes #752 bug/252-Fix checkbox not updating by abstracting state away from component renders closes #252 Apr 18, 2020
@jordansimsmith
Copy link
Contributor

Hey @preetpatel, can you elaborate on why this is a fix? It looks functionally equivalent to me.

@HarrisonLeach1
Copy link
Contributor

HarrisonLeach1 commented Apr 19, 2020 •
edited
Loading

Hey @preetpatel, can you elaborate on why this is a fix? It looks functionally equivalent to me.

I agree, the behaviour from #252 can still be replicated. The checkbox should work without having to refresh the page.

@jordansimsmith jordansimsmith self-assigned this Apr 22, 2020
@jordansimsmith
Copy link
Contributor

Closing for now, feel free to reopen when its ready to be merged in :)

@preetpatel
Copy link
Contributor Author

Rip.. just started working on it haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harmanlamba harmanlamba harmanlamba approved these changes

@HarrisonLeach1 HarrisonLeach1 Awaiting requested review from HarrisonLeach1

@Matteas-Eden Matteas-Eden Awaiting requested review from Matteas-Eden

@EricPedrido EricPedrido Awaiting requested review from EricPedrido

Assignees

@jordansimsmith jordansimsmith

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Checkbox in to-do list items doesn't update correctly
5 participants
@preetpatel @EricPedrido @jordansimsmith @HarrisonLeach1 @harmanlamba