-
Notifications
You must be signed in to change notification settings - Fork 414
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
Video overlay #825
Video overlay #825
Conversation
687c521
to
1da82cc
Compare
return ( | ||
<div className="video__loading-screen"> | ||
<div> | ||
{spinner && <div className="video__loading-spinner" />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the <Spinner />
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed one thing. We already have a common.js
file here(file 1), but the <Spinner />
component comes from here(folder 1), a new common folder with just that component.
Are we breaking down file 1 to individual components and putting them in folder 1? Or should I include <Spinner />
component in file 1. The latter option is better IMO as then we don't have to include different files for each "common" component, unless former is better for some use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hackrush01 @seanyesmunt @liamcardenas I think common.js
should die and these should all just be individual components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
This is on hold. When we're ready to work on this, we should re-evaluate and figure out the right approach. |
No description provided.