search

lhz516 / react-h5-audio-player

React audio player component with UI. It provides time indicator on both desktop and mobile devices.
https://codepen.io/lhz516/pen/dyGpmgP
MIT License
608 stars 92 forks source link

playing and paused classes are flipped #132

Closed ptmkenny closed 2 years ago

ptmkenny commented 2 years ago

Hey, I'm very sorry about this, but upon testing the new release, I realized I flipped the playing and paused classes. 😥 This PR fixes that.

lhz516 commented 2 years ago

The class name is more like button status, not audio status.

When the audio is playing, we are seeing the button with Paused icon. When the audio is paused, we are seeing the button with Play icon.

We can update the documentation, and not necessary to change the class name. I'm concerned that changing the naming after publishing README is a breaking change. Let me know your thoughts.

ptmkenny commented 2 years ago

I was thinking that the class name is the audio status, so I assigned the names playing and paused.

If the class name is supposed to be the button status, then it would be better to name the classes play and pause.

playing indicates "the audio is playing now", and paused indicates "the audio is paused now" (so playing and paused suggest that the class describes the audio status).

On the other hand, play indicates "press to play", and pause indicates "press to pause" (so play and pause would probably be better names if it is the button status).

lhz516 commented 2 years ago

Ok, I agree. I'm merging this. Let's try not to bring breaking changes in the future XD.

lhz516 commented 2 years ago

Actually let's consider this as a bug fix, so it's no longer a breaking change.

ptmkenny commented 2 years ago

Yeah, I'll do my best to be more careful.

I agree it's a bug fix because the classes didn't do what their names suggested.