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

Play button fixed #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Play button fixed #47

wants to merge 2 commits into from

Conversation

jaadez
Copy link

@jaadez jaadez commented Jul 15, 2024

Added and tweaked a few css styles within audioControls.scss. changes were made to audio-controls and play-btn components.

… were made to audio-controls and play-btn components.
Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

This does the job but I'd like to provide some high-level feedback for a few items:

  • Commenting out code or styles is fine during development, but prior to committing you should determine if those commented items need to remain or should be removed. Ideally the styles that were commented out in audioControls.scss are fully removed if they are no longer needed. If we're leaving commented code in, a comment above the block describing the reason is preferable.
  • Avoid random updates to the yarn lockfile if possible, especially in unrelated pull requests. There are a couple reasons your lockfile might update or regenerate, including differences in node versions, removing/regenerating a lockfile to resolve a package issue, etc. It's generally not a big deal but it pollutes the diff and could potentially cause issues if dependencies were accidentally rolled back to earlier versions. That doesn't appear to be the case here, thankfully.
  • Naming your branch isn't a huge issue, but we generally like to see branches named using the following schema: issue/<issue number>-slug-describing-issue. For example, this branch might've been named issue/46-fix-creator-audio-play-button. In your defense the rest of the cohort doesn't really adhere to this either.

One other thing I'd like to see: the seek bar is right up against the play button:
Screenshot 2024-07-25 at 1 03 27 PM

I'd like to see it better aligned within the space reserved for the audio controls element. The easiest solution would probably be to apply flexbox styling to the audioControls element itself.

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.

2 participants