Refactor TimerServiceProvider to no longer use withExamStore higher-order-component. #133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2u/cosmonauts/refactor-store #133 +/- ##
=============================================================
Coverage 93.20% 93.20%
=============================================================
Files 71 71
Lines 1060 1060
Branches 291 291
=============================================================
Hits 988 988
Misses 67 67
Partials 5 5 ☔ View full report in Codecov by Sentry. |
alangsto
reviewed
Feb 2, 2024
| expireExam, pollAttempt, apiErrorMsg, pingAttempt, | ||
| getLatestAttemptData, | ||
| } = state; | ||
| const { showTimer } = examState; |
There was a problem hiding this comment.
showTimer is only available if ExamStateProvider is being used, which we want to move away from. I did some of this clean up in #132, but I like how you removed all the props being passed down to the child Timer components.
We might need to do a mix of both your changes and my changes.
alangsto
reviewed
Feb 2, 2024
| isLoading, activeAttempt, showTimer, stopExam, exam, | ||
| expireExam, pollAttempt, apiErrorMsg, pingAttempt, | ||
| getProctoringSettings, submitExam, | ||
| isLoading, showTimer, exam, apiErrorMsg, getProctoringSettings, |
There was a problem hiding this comment.
Same issue with showTimer here
There was a problem hiding this comment.
Although maybe it's not an issue? I just am not sure why we need ExamStateContext anymore
There was a problem hiding this comment.
Just reiterating our conversation from standup for posterity, but I was just trying not to interfere with the files you were working on. I agree that we can move showTimer into the components that need it and get rid of ExamStateProvider!
varshamenon4
approved these changes
Feb 2, 2024
| import { ExamTimerBlock } from '../timer'; | ||
| import ExamAPIError from '../exam/ExamAPIError'; | ||
| import ExamStateProvider from './ExamStateProvider'; | ||
|
|
||
| const ExamTimer = ({ courseId }) => { | ||
| const state = useContext(ExamStateContext); | ||
| const examState = useContext(ExamStateContext); |
There was a problem hiding this comment.
Nice! I like the specificity of using examState here.
| const { authenticatedUser } = useContext(AppContext); | ||
| const { |
There was a problem hiding this comment.
[question] Do we just no longer need all of these consts from state?
There was a problem hiding this comment.
If you take a look at where these were being used in this component, they were just being passed as props to ExamTimerBlock and weren't being used anywhere else (except showTimer).
I've refactored ExamTimerBlock to pull these values either out of Redux (if they're state) or import them out of the data module (if they're thunks), which means ExamTimerBlock no longer needs to get these values as props. OuterExamTimer was getting these as props from its parent and doing some prop drilling to send them to ExamTimerBlock, but since ExamTimerBlock can now get what it needs itself, it no longer needs OuterExamTimer to send it those props.
Base automatically changed from
michaelroytman/COSMO-173-refactor-store-instructions
to
2u/cosmonauts/refactor-store
February 2, 2024 16:29
…rServiceProvider This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks.
MichaelRoytman
force-pushed
the
michaelroytman/COSMO-174-refactor-timer-provider
branch
from
e52c1a3 to
10ee30d
Compare
MichaelRoytman
deleted the
michaelroytman/COSMO-174-refactor-timer-provider
branch
alangsto
pushed a commit
that referenced
this pull request
Feb 8, 2024
… useSelector hooks API. (#131) * feat: rename examStore to specialExams in preparation for use in frontend-app-learning This commit renames the examStore to specialExams. This is in preparation for the use of the exam reducer in the frontend-app-learning React application. * refactor: replace use of context with thunks and Redux store in instructions components This commit replaces the use of the ExamStateContext with the use of thunks and the Redux store in the instructions components. The original pattern was to use the withExamStore higher-order component to provide context to the instructions components. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. * test: remove references to ExamStateProvider from tests for instructions components This commit removes references to the ExamStateProvider from tests for instructions components. Because these components have been refactored to no longe rely on the ExamStateProvider, they are not required in the component tree. refactor: replace use of withExamStore higher-order component in TimerServiceProvider (#133) This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. feat: refactor non instruction components (#132) Refactor public API functions to no longer import and use store. (#134) * test: remove asynchronicity from initializing test store The initializeTestStore test utility function used async...await keywords for initializing the test store, which isn't necessary. This commit removes the async...await keywords and refactors any tests that use that function. * test: fix mocking of getExamAttemptsData function * feat: refactor public API to use hooks instead of references to exported store This commit refactors the public API, used by the frontend-app-learning application to interact with the frontend-lib-special-exams state, to export a series of hooks. Originally, the public API imported the frontend-lib-special-exams store directly and operated on it in a series of exported functions. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by public API. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. This commit also exports the root reducer from this library. This root reducer will be imported by the frontend-app-learning application and used to configure its store. fix: patch bugs found during bash (#135)
alangsto
pushed a commit
that referenced
this pull request
Feb 8, 2024
… useSelector hooks API. (#131) * feat: rename examStore to specialExams in preparation for use in frontend-app-learning This commit renames the examStore to specialExams. This is in preparation for the use of the exam reducer in the frontend-app-learning React application. * refactor: replace use of context with thunks and Redux store in instructions components This commit replaces the use of the ExamStateContext with the use of thunks and the Redux store in the instructions components. The original pattern was to use the withExamStore higher-order component to provide context to the instructions components. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. * test: remove references to ExamStateProvider from tests for instructions components This commit removes references to the ExamStateProvider from tests for instructions components. Because these components have been refactored to no longe rely on the ExamStateProvider, they are not required in the component tree. refactor: replace use of withExamStore higher-order component in TimerServiceProvider (#133) This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. feat: refactor non instruction components (#132) Refactor public API functions to no longer import and use store. (#134) * test: remove asynchronicity from initializing test store The initializeTestStore test utility function used async...await keywords for initializing the test store, which isn't necessary. This commit removes the async...await keywords and refactors any tests that use that function. * test: fix mocking of getExamAttemptsData function * feat: refactor public API to use hooks instead of references to exported store This commit refactors the public API, used by the frontend-app-learning application to interact with the frontend-lib-special-exams state, to export a series of hooks. Originally, the public API imported the frontend-lib-special-exams store directly and operated on it in a series of exported functions. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by public API. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. This commit also exports the root reducer from this library. This root reducer will be imported by the frontend-app-learning application and used to configure its store. fix: patch bugs found during bash (#135)
alangsto
added a commit
that referenced
this pull request
Feb 8, 2024
… useSelector hooks API. (#131) (#136) * feat: rename examStore to specialExams in preparation for use in frontend-app-learning This commit renames the examStore to specialExams. This is in preparation for the use of the exam reducer in the frontend-app-learning React application. * refactor: replace use of context with thunks and Redux store in instructions components This commit replaces the use of the ExamStateContext with the use of thunks and the Redux store in the instructions components. The original pattern was to use the withExamStore higher-order component to provide context to the instructions components. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. * test: remove references to ExamStateProvider from tests for instructions components This commit removes references to the ExamStateProvider from tests for instructions components. Because these components have been refactored to no longe rely on the ExamStateProvider, they are not required in the component tree. refactor: replace use of withExamStore higher-order component in TimerServiceProvider (#133) This commit replaces the use of the withExamStore higher-order component with the useDispatch and useSelector hooks in TimerServiceProvider. This commit also refactors components that use the TimerServiceProvider so that they no longer need to pass state and action creators via props. The TimerServiceProvider now gets whatever state it needs directly from the Redux store with a useSelector hook and imports and dispatches thunks directly by importing them from the data directory. The original pattern was to use the withExamStore higher-order component to provide context to the TimerServiceProvider and its children. This context contained provided the Redux store state and action creators as props by using the connect API. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by the higher-order component. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. feat: refactor non instruction components (#132) Refactor public API functions to no longer import and use store. (#134) * test: remove asynchronicity from initializing test store The initializeTestStore test utility function used async...await keywords for initializing the test store, which isn't necessary. This commit removes the async...await keywords and refactors any tests that use that function. * test: fix mocking of getExamAttemptsData function * feat: refactor public API to use hooks instead of references to exported store This commit refactors the public API, used by the frontend-app-learning application to interact with the frontend-lib-special-exams state, to export a series of hooks. Originally, the public API imported the frontend-lib-special-exams store directly and operated on it in a series of exported functions. This posed a problem for our need to merge the frontend-app-learning and frontend-lib-special-exams stores, because the special exams store is initialized in this repository and used by public API. In order to eventually be able to remove the creation of the store in this repository, we have to remove references to the store by interfacing with the Redux more directly by using the useDispatch and useSelector hooks. This commit also exports the root reducer from this library. This root reducer will be imported by the frontend-app-learning application and used to configure its store. fix: patch bugs found during bash (#135) Co-authored-by: Michael Roytman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Jira: COSMO-174