Skip to content
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

VideoSection can be refactored #1341

Open
aryanpingle opened this issue Dec 23, 2023 · 5 comments
Open

VideoSection can be refactored #1341

aryanpingle opened this issue Dec 23, 2023 · 5 comments

Comments

@aryanpingle
Copy link
Contributor

I've listed some bugs below, but this is mostly a critique of the code structure which is the overarching problem.

I was trying to fix a small issue in #1339 that requires setting the background of the VideoSection component's .timeline. But there are 2 independent, yet similar implementations of VideoSection for Tracks and Challenges - this required changing 2 files for a feature that is common to both.

In fact, challenges/VideoSection.js and tracks/VideoSection.js are mostly the same except for a few variables and what happens when a "Track Stop" or a "Part" is clicked. Their respective CSS files are have even more similarity, differing in hardly a few lines (and these may be unintentional).

Is this important? Well yeah, because code redundancy has already caused some differences:

Challenges Tracks
image image
image image
image image

Potential Solution

If we refactor VideoSection to be a Class Component instead of a Functional Component, we can inherit from a more general VideoSection that'll make the code more readable and DRY. The CSS files have practically no differences because we pass in the variant programmatically for UI changes. They can be merged, and we can use this merged file for the base VideoSection class.

@aryanpingle
Copy link
Contributor Author

Another separate issue that arises because of the code duplication is that timestamps aren't left-padded with zeroes in Challenges:

Challenges Tracks
image image

I suppose we just need to add a line somewhere in challenges/VideoSection.js, but refactoring the code will help us easily fix this anyway.

(Also I just wanna mention these are minor nitpicks, this codebase is generally excellently structured LOL)

@loic-brtd
Copy link
Contributor

Another separate issue that arises because of the code duplication is that timestamps aren't left-padded with zeroes in Challenges

@aryanpingle Regarding timestamps, I think they're directly taken from the JSON files, without any transformation. So the differences come from the JSON files themselves.

content/videos/challenges/171-wave-function-collapse/index.json :

  "timestamps": [
    { "time": "0:00", "title": "Day 1! Wave Function Collapse!" },
    { "time": "2:00", "title": "Entropy in Sudoku." },

content/videos/webgl/3-light-and-material/index.json :

  "timestamps": [
    { "time": "00:00", "title": "Introduction" },
    { "time": "00:45", "title": "Material is the \"skin\" of 3D object" },

@shiffman
Copy link
Member

Thank you @aryanpingle for filing this and @loic-brtd for your response! I would love to streamline the code and data format as much as possible for the site! @fturmel and @dipamsen do you have any thoughts about this?

@fturmel
Copy link
Collaborator

fturmel commented Mar 20, 2024

Sure, it could be worth an attempt at merging/refactoring these two components (as long as we find wind a way to not dramatically increase cyclomatic complexity because we're chasing DRY too hard).

Are the CSS differences actual bugs though? Challenges don't have h2s, for example.

@fturmel
Copy link
Collaborator

fturmel commented Mar 20, 2024

For the timestamps, I assume we want the zero padding enforced?

The build step already computes the timestamp representation in seconds, so we could format this value with the correct padding in React.

Or, we could update the JSON sources (it can be automated) and be stricter about the format in the content tests. Less redundant compute down the road. This might affect (positively?) the youtube description generator script as well.

EDIT: I suppose a third option is handling the formatting cleanup at the Gatsby build step. The YouTube description generator reads from the JSON source though if that matters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants