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

Add persistor to pset components #115

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

MReyna12
Copy link
Contributor

No description provided.

@MReyna12 MReyna12 marked this pull request as ready for review September 22, 2023 16:54
@MReyna12
Copy link
Contributor Author

Notes:

  1. For the multiselect interactive: In order to use an array inside of the useEffect dependency i wrapped it with JSON.stringify(). Per this discussion it looks like this is a viable option in our scenario, but I am open to another solution either outlined in the discussion or somewhere else.
  2. All PSET interactives:
    a. Error handling on failed put - Currently, I am only using console.error(err). Because the put occurs outside of the handleSubmit, I do not think I can use setFieldError in the same manner as UserInputBlock.tsx. Any ideas on how to handle a put failure?
    b. I believe we need retriesAllowed and formDisabled/inputDisabled as a dependency of the useEffect. I've also included response as a dependency, but response updating occurs when retriesAllowed and formDisabled/inputDisabled updates as well. Should we include response as a dependency or would retriesAllowed and formDisabled/inputDisabled be sufficient?

@MReyna12 MReyna12 requested a review from a team September 22, 2023 17:05
Copy link
Contributor

@rnathuji rnathuji left a comment

Choose a reason for hiding this comment

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

@MReyna12 - This is a partial review of just ProblemSetBlock and InputProblem. The reason being, depending on thoughts there it would impact the other components in a similar way. See what you think about my thoughts, but I believe they would address both of the items you raised here:

  • Item 1: It would remove the pattern of using state as in the useEffect dependencies altogether
  • Item 2: The .put() would get moved into handleSubmit() so we could employ the same error handling pattern we have in UserInputBlock.

src/components/ProblemSetBlock.tsx Outdated Show resolved Hide resolved
src/components/InputProblem.tsx Outdated Show resolved Hide resolved
src/components/InputProblem.tsx Show resolved Hide resolved
src/components/InputProblem.tsx Outdated Show resolved Hide resolved
@MReyna12
Copy link
Contributor Author

@rnathuji - My latest commit addresses your comments. I am still trying to see if there is anything that can be done in the submit handler to not effectively write the same try/catch statement three times (unless you think it is not necessary). Once these updates get approval I will make similar updates to the other components.

Copy link
Contributor

@rnathuji rnathuji left a comment

Choose a reason for hiding this comment

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

@MReyna12 - I think the changes to InputProblem in general look good and can be replicated across. I don't see an obvious way to avoid the multiple try / catch statements, and perhaps that's something we can just defer to a future "real" implementation since what you have does address the key pieces. I did have a few comments on the new handleFeedback().

src/components/InputProblem.tsx Outdated Show resolved Hide resolved
src/components/InputProblem.tsx Outdated Show resolved Hide resolved
src/components/InputProblem.tsx Outdated Show resolved Hide resolved
src/components/InputProblem.tsx Outdated Show resolved Hide resolved
@MReyna12 MReyna12 merged commit ce3d3f9 into k12-419/persist-ib Sep 27, 2023
@MReyna12 MReyna12 deleted the k12-513/Extend-persistor-pset-components branch September 27, 2023 20:46
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.

2 participants