Refactor of display logic in preparation for adding default dropdown behavior #42712
Conversation
jmkulwik
changed the base branch from
staging
to
ben/code-review/move-token-into-data-api
| @@ -70,6 +70,7 @@ class ReviewTab extends Component { | |||
| serverLevelId, | |||
| serverScriptId | |||
| } = getStore().getState().pageConstants; | |||
There was a problem hiding this comment.
may be out of scope, but would you mind moving these to be passed in as props via redux?
There was a problem hiding this comment.
Curious about this in general -- since these are constants (ie, shouldn't change), do we still prefer they be passed in as props?
There was a problem hiding this comment.
From what I've seen, getStore.getState generally only gets called for libraries that can't be connected. React components generally get values like this by being connected and passing via props. I don't know the exact reason why we don't tend to call getStore.getState from within a component, but I'd generally be surprised to find something controlling a component by anything besides props.
There was a problem hiding this comment.
I have one more follow-up in the works after the functional change. I'll plan to do this as part of that change unless someone else gets to it before me.
| } | ||
|
|
||
| return ( | ||
| <> |
|
|
||
| return ( | ||
| <div style={styles.reviewsContainer}> | ||
| <div style={styles.reviewHeader}> |
There was a problem hiding this comment.
Nit: do we need this div if header is empty? Could we put it in renderHeader() if not?
There was a problem hiding this comment.
My follow-up PR (in progress) moves things around a bit more. I ended up getting rid of renderHeader. So I'll leave this as-is for now as it gets cleaned up in the next one:
https://github.com/code-dot-org/code-dot-org/pull/42716/files
jmkulwik
changed the base branch from
ben/code-review/move-token-into-data-api
to
staging
jmkulwik
force-pushed
the
jessie-prefactor
branch
from
3c62c5c
to
76cf867
Compare
jmkulwik
changed the base branch from
staging
to
ben/code-review/move-token-into-data-api
This does a few things:
viewAs === ViewType.Teacherto aviewAsTeacherpropstateandpropsvalues anywhere with more than one reference to state/propsstateandpropsnamescodeReviewEnabledand!viewAsTeacherare only checked onceLinks
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: