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

#1544 Fix: Play/Pause Button Centered #1556

Closed
wants to merge 2 commits into from
Closed

#1544 Fix: Play/Pause Button Centered #1556

wants to merge 2 commits into from

Conversation

amanbhardwaj12072003
Copy link

  • Fix: #1544

  • Bug: The Play/Pause button at the bottom of the window isn't centered between the Previous and Next buttons - it's closer to the Next button (i.e. to the right). Hard to see for the Play button, but easier with the Pause button:

  • Change: Added margin to the right of the button, as suggested by @nukeop in his comment regarding the issue.

@nuki-chan
Copy link

nuki-chan bot commented Feb 20, 2024

Lines 1-9

Ah, I see we're having the classic package-lock.json party where the only guest is a version bump! 🥳 If it were up to Nuki, we'd throw confetti every time, but seriously, unless you changed your package.json dependencies, why are you settling for this lonely update in the giant lock file? Be careful with this; unwanted changes here could wreak havoc in Nuki land (a.k.a. your project). Before committing package-lock.json, always check if the dependencies in package.json were actually changed, okay?

No code changes needed considering there are actual dependency updates in package.json. If not, this change should not be part of your PR.


Lines 22-37

Oh, the classic case of style-margin instead of using good ol' CSS classes. Nuki is not amused. 😒 Using inline styles is like eating sushi with a fork - sure, you can do it, but it's far from best practice! You should create a beautiful class in your CSS that handles your margins, then add it to your className like it's the most kawaii thing ever 😍.

Here's a suggestion to keep things tidy:

// Add a new class in your CSS module
.player_button_with_margin {
  margin-right: 4px;
}

And then, use it like this:

// Line 22-37
}) => (
  <button
    data-testid={rest['data-testid']}
    className={cx(
      styles.player_button,
      { [styles.disabled]: disabled },
      { [styles.player_button_with_margin]: rest['data-testid'] === 'player-controls-play' } // Apply the margin conditionally based on test-id if needed
    )}
    aria-label={ariaLabel}
    onClick={disabled ? undefined : onClick}
  >
    <Icon inverted loading={loading} name={icon} size={size} />
  </button>
);

This way, your styles stay in the world of CSS where they belong, and every anime fan is happy. (∩^o^)⊃━☆゚.*


Lines 36-42

Uh oh, what's this? An inline style slipping into our pristine JSX? Absolutely not on Nuki's watch! Remove this like you would an incorrectly subtitled translation ruining the mood of a pivotal scene. 😤

Update your player button with the recommended changes from above to manage margins with class and style, turning your code from I guess it works to absolute masterpiece. Trust me, your users will feel the harmony zen. ✨

<PlayerButton
  data-testid='player-controls-play'
  loading={isLoading}
  icon={

This line should not contain any inline styles. Use CSS classes.

@nukeop
Copy link
Owner

nukeop commented Feb 20, 2024

I agree with Nuki, don't commit your package-lock.json, and don't use style. Instead, you should add a new class to the scss, and apply it. Don't use data-test for applying styles like Nuki suggets though, that's wrong.

@amanbhardwaj12072003
Copy link
Author

Hi @nukeop, can you please approve my PR.

className={cx(
styles.player_button,
{ [styles.disabled]: disabled },
{ [styles.player_button_with_margin]: rest['data-testid'] === 'player-controls-play' } // Apply the margin conditionally based on test-id if needed
Copy link
Owner

Choose a reason for hiding this comment

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

That's not the right way, data-testid is only for tests, it should not influence business logic in any way.

@nukeop nukeop added the needs changes The author needs to make changes to this pull request. label Feb 24, 2024
@amanbhardwaj12072003 amanbhardwaj12072003 closed this by deleting the head repository Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changes The author needs to make changes to this pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants