Refactor media player #183
Conversation
|
@jamesdools given that in the last refactor we introduced a bug #185 (now fixed #186) I suggest that before going ahead with this the QA doc should be updated, and QA should be run as an acceptance criteria before merging PRs that do substantial refactor. #187 |
jamesdools
force-pushed
the
refactor-media-player
branch
from
b77f25a
to
9557faa
Compare
|
Done the QA on this - looks okay to me. |
| @@ -6,36 +6,28 @@ module.exports = { | |||
| module: { | |||
| rules: [ | |||
| { | |||
| test: /\.module.css$/, | |||
There was a problem hiding this comment.
does this mean we are only supporting CSS modules?
demo/index.module.scss
Outdated
|
|
||
| .container { | ||
| height: 100vh; | ||
| font-family: ReithSerif, Fallback, sans-serif; |
There was a problem hiding this comment.
I don't think Fallback will resolve to anything - is this meant to be a reference to a mixin?
There was a problem hiding this comment.
Good catch - I followed this from somewhere (I'm sure int-gel-matter or a component library) but I can't find it anymore - it's only us doing it!
So yeah - I think this can be removed 馃槄
demo/index.module.scss
Outdated
| } | ||
|
|
||
| /* desktop */ | ||
| @media (min-width: 767px) { |
There was a problem hiding this comment.
| @media (min-width: 767px) { | |
| @media (min-width: 768px) { |
Breakpoints can go a bit weird if the min/max are the same - better for the min-width to be max+1
There was a problem hiding this comment.
Ah yeah, I'm never quite sure on this. So we should move to 768?
|
|
||
| return false; | ||
| shouldComponentUpdate = (nextProps) => { | ||
| return nextProps !== this.props; |
There was a problem hiding this comment.
I think checking for object equality here will always return true (since it only checks that they're the same reference in memory). Better to compare on the relevant props that might have changed between renders.
There was a problem hiding this comment.
Hmm. Yeah this was to improve the rendering performance but without cluttering the code too much. Is there a working one-liner for this?
There was a problem hiding this comment.
Thinking either the lodash isEqual or perhaps this:
https://www.npmjs.com/package/react-fast-compare
have you used it before?
| if (nextProps !== this.props) return true; | ||
|
|
||
| return false; | ||
| return nextProps !== this.props; |
There was a problem hiding this comment.
Same comment as above ^ re: checking props equality
Is your Pull Request request related to another issue in this repository ?
Describe what the PR does
Refactoring the codebase, starting with MediaPlayer. Initially making the methods and props more succinct, will add testing and optimisations later.
Going by commit:
if videoRef !== nullchecks.State whether the PR is ready for review or whether it needs extra work
馃憤
Additional context