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

Extend the constructColor function to handle 6 digit hex codes #5238

Merged

Conversation

practual
Copy link
Contributor

@practual practual commented Jun 8, 2018

Description

When providing the TextTrackDisplay component with track settings overrides, currently colour codes with only three digits are supported. I would like to make this more flexible by also allowing six digit hex codes.

CC @arski

Specific Changes proposed

Updates the TextTrackDisplay component's constructColor function to accept and process both three and six digit colour codes.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@arski
Copy link
Contributor

arski commented Jun 8, 2018

nice, thanks. @gkatsev could you take a look please if you could include this

@OwenEdwards
Copy link
Member

I'd like to see unit tests on this, to verify that both color formats work. I've got some work coming up in this same area, and I'd need to merge it with this change, so I'd want to make sure that I didn't break anything in the merge.

@practual
Copy link
Contributor Author

@OwenEdwards tests added.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Other than the rest of the code base not using switches, this looks good to me.

@OwenEdwards
Copy link
Member

@gkatsev is there a downside to switchs, rather than if...else...?

@gkatsev
Copy link
Member

gkatsev commented Jun 15, 2018

You definitely have to be more careful with switches due to their auto fall through and default case.

@gkatsev
Copy link
Member

gkatsev commented Jun 15, 2018

Also, it doesn't match the rest of our code :)

@practual
Copy link
Contributor Author

I find switch statements cleaner than a lot of else ifs for the same variable, but will change it if you want.

@practual
Copy link
Contributor Author

@gkatsev ping.

@gkatsev
Copy link
Member

gkatsev commented Jun 20, 2018

I think I would prefer it to be if statements over a switch for consistency.

@practual
Copy link
Contributor Author

@gkatsev updated.

@gkatsev
Copy link
Member

gkatsev commented Jun 21, 2018

Thanks @practual

@gkatsev gkatsev merged commit e92f177 into videojs:master Jun 21, 2018
@practual practual deleted the extend-text-track-display-color-code-support branch June 21, 2018 22:54
gkatsev pushed a commit that referenced this pull request Jul 9, 2018
…e 6 digit hex codes (#5238)

When providing the TextTrackDisplay component with track settings overrides, currently colour codes with only three digits are supported. This updates it so that 6 digit hex codes are also accepted.
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.

5 participants