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

Fix audio track number not displayed and adds audio disk (media) number #1454

Merged
merged 6 commits into from
Sep 25, 2018

Conversation

Borewit
Copy link
Member

@Borewit Borewit commented Aug 15, 2018

  • Fixes audio track number not displayed of the total number of is not defined
  • Adds the disk (media) number to audio track properties
  • Changes the order of audio track properties

@Borewit
Copy link
Member Author

Borewit commented Aug 15, 2018

Disk number, total nr of disks, track nr & total tracks displayed:
track disk nr - parov stelar - the princess

In this track, only the track number is present:
track disk nr - robin grey - these days

Copy link
Contributor

@codealchemist codealchemist left a comment

Choose a reason for hiding this comment

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

This is a nice enhancement! Thanks!

if (common[key].of) {
str += ` of ${common[key].of}`
}
return React.createElement('div', { key, className: 'audio-' + key },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest returning a JSX block, like the one we had before.

// Audio metadata: disk & track-number
const count = ['track', 'disk']
count.forEach(key => {
const nrElem = renderTrack(common, key, key[0].toUpperCase() + key.substring(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use CSS to capitalize the label.
If we do that we would only pass key and avoid operating directly on the string.

On renderTrack we would be able to do:

    const style = { textTransform: 'capitalize' }
    return (
      <div className={`audio-${key}`}>
        <label style={style}>{key}</label> {str}
      </div>
    )

Copy link
Member Author

@Borewit Borewit Aug 18, 2018

Choose a reason for hiding this comment

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

I have no experience with React, I blindly followed your suggestion.
But seems to do the job, so well done.

// Audio metadata: disk & track-number
const count = ['track', 'disk']
count.forEach(key => {
const nrElem = renderTrack(common, key, key[0].toUpperCase() + key.substring(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we can remove the third param, which is not needed anymore.
:)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yeah sure.

@Borewit Borewit added the meta label Aug 19, 2018
@codealchemist
Copy link
Contributor

Found this error when common[key] is undefined on renderTrack:
image

Copy link
Contributor

@codealchemist codealchemist left a comment

Choose a reason for hiding this comment

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

Just did some quick testing and added a fix to handle undefined values on renderTrack.
I think we're ready to merge :)

@Borewit
Copy link
Member Author

Borewit commented Aug 19, 2018

Good catch, but strange, common.track: & common.disk should always exist...

  public readonly common: ICommonTagsResult = {
    track: {no: null, of: null},
    disk: {no: null, of: null}
  };

Ref

@codealchemist
Copy link
Contributor

On player-page:232 we default common to {} when falsy.
The first two calls to renderTrack were made with common not being set yet:
image

Copy link
Contributor

@mathiasvr mathiasvr left a comment

Choose a reason for hiding this comment

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

LGTM, lets get this merged 👍

@mathiasvr mathiasvr merged commit d40449f into master Sep 25, 2018
@mathiasvr mathiasvr deleted the fix/audio-track-nr branch September 26, 2018 21:11
@lock lock bot locked as resolved and limited conversation to collaborators Dec 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants