Skip to content

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Feb 15, 2025

Important

Add a tutorial videos section with a new JSON data file, MDX page, React component, and CSS styling.

  • New Feature:
    • Adds tutorial-videos.json to store video data with id and title.
    • Creates tutorial-videos.mdx to introduce the tutorial videos section using VideoGrid component.
    • Updates sidebars.ts to include tutorial-videos in the sidebar.
  • Components:
    • Adds VideoGrid.tsx to render a grid of videos using data from tutorial-videos.json.
    • Implements click-to-play functionality for videos using YouTube embeds.
  • Styling:
    • Adds CSS styles in custom.css for .video-grid, .video-container, and .video-title to format the video display.

This description was created by Ellipsis for ece2edf. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to ece2edf in 2 minutes and 9 seconds

More details
  • Looked at 194 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. docs/tutorial-videos.mdx:7
  • Draft comment:
    Add a newline at end of file for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While having a newline at end of file is a common convention, this is exactly the kind of thing that should be handled by ESLint/Prettier automation rather than manual code review comments. Our standards specifically mention that ESLint and Prettier should be set up to enforce code style rules automatically.
    The comment is technically correct about a common convention. Maybe enforcing this manually is important if the automation isn't set up yet?
    No - our standards explicitly state that ESLint/Prettier should be set up to handle code style rules automatically. Making manual comments about things that should be automated doesn't add value.
    Delete this comment as it addresses a code style issue that should be handled by automated tooling rather than manual review.
2. src/components/VideoGrid.tsx:18
  • Draft comment:
    Avoid using direct DOM manipulation with innerHTML in React—this can bypass the virtual DOM and pose XSS risks. Instead, use state to conditionally render the <iframe>.
  • Reason this comment was not posted:
    Marked as duplicate.
3. src/components/VideoGrid.tsx:16
  • Draft comment:
    Consider extracting the inline onClick handler into a separate function to improve readability and testability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. docs/tutorial-videos.mdx:7
  • Draft comment:
    Add a newline at the end of file to comply with typical Prettier/ESLint settings.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_6gmz7E2qe675bCW6


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

className="video-container"
onClick={(e) => {
const target = e.currentTarget;
target.innerHTML = `<iframe src='https://www.youtube.com/embed/${video.id}?autoplay=1' title='${video.title}' allow='accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture' allowFullScreen></iframe>`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using innerHTML injection in the onClick handler. Use component state for conditional rendering of the iframe to prevent potential XSS risks and ensure React-managed DOM updates.

<div className="video-grid">
{videos.videos.map((video: Video) => (
<div key={video.id} className="video-item">
<div
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance accessibility by adding attributes (e.g., role='button', tabIndex=0) to the clickable video container.

Suggested change
<div
<div role="button" tabIndex={0}

@mrubens mrubens merged commit fe99223 into main Feb 15, 2025
2 checks passed
@mrubens mrubens deleted the tutorial_videos branch February 15, 2025 05:31
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

Successfully merging this pull request may close these issues.

1 participant