-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Reformatting and Refactoring #5138
base: master
Are you sure you want to change the base?
Conversation
hey @AHOHNMYC @ChunkyProgrammer, could you please take a look at this PR? You have more JS experience than me :). thanks |
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.
Changes looks good. I just have one comment 🙂
assets/js/_helpers.js
Outdated
if (matchedCookie) { | ||
const cookieBody = matchedCookie.replace(cookiePrefix, ''); | ||
if (cookieBody.length === 0) return; | ||
if (!cookieBody.length) return; |
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.
cookieBody.length === 0 is better for readability so I'd recommend keeping that. If we want to use less bytes for this type of statement, i think a minifier should be used instead.
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.
Good call. We can minify later as desired.
Added addition for displaying a message at the comments section if comments for the video were turned off. The current behavior does not display anything if comments don't load, which can be confusing. |
It seems this functionality was already attempted but was never merged: #4051 |
Please split that into a separate PR. Keep this PR originally for js refactoring and create a new PR for fixing specifically for the other fix. |
…these changes to another PR This reverts commit 5451391.
This is my first open source contribution. I thought I would start simple with some basic formatting and refactoring. Thank you.