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

Stream Marquee #206

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

Stream Marquee #206

wants to merge 26 commits into from

Conversation

jdliaw
Copy link
Contributor

@jdliaw jdliaw commented Feb 1, 2018

Types of changes

  • Bugfix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Refactor (change which changes the codebase without affecting its external behavior)
  • Non-breaking change (fix or feature that would causes existing functionality to work as expected)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Purpose

Add a marquee to the stream bar to post announcements, on-air giveaways, or display the on-air/request line numbers, etc.

Approach

Created a new StreamMarquee component that takes in a message as props and uses webkit-animation to move and look nice.

Screenshot(s)

img

Checklist

  • My branch follows the branch naming scheme of UCLA Radio, and can merge into master without error.
  • My code follows the code style of this project, and I have linted it to confirm this.
  • I have added tests that prove my fix is effective or that my feature works.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • All new and existing tests passed.

'https://ws.audioscrobbler.com/2.0/?method=user.getRecentTracks&user=uclaradio&api_key=d3e63e89b35e60885c944fe9b7341b76&limit=10&format=json';
const streamURL = 'http://uclaradio.com:8000/;';
let stream;
var streamURL = 'http://uclaradio.com:8000/;';
Copy link
Member

Choose a reason for hiding this comment

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

Please use const and let over var. I think this happens in multiple files.

const RecentlyPlayed = React.createClass({
getInitialState() {
var RecentlyPlayed = React.createClass({
getInitialState: function() {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between declaring a class method as:

getInitialState() {

vs

getInitialState: function() {

I haven't seen the latter notation before!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh same idk where any of these changes came from cuz i didn't change any of these lines for the stream marquee but ima change them back!!!

@@ -123,7 +135,7 @@ const StreamBar = React.createClass({
</span>
<span className="playText">
{this.props.currentShowTitle ? (
`LIVE: ${this.props.currentShowTitle}`
'LIVE: ' + this.props.currentShowTitle
Copy link
Member

Choose a reason for hiding this comment

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

Please prefer string template literals to concatenation.

Copy link
Member

@binerys binerys left a comment

Choose a reason for hiding this comment

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

image

^ Text is going over the stream logo. I'm on Chrome if that helps!

Also, can we make the marquee move a little a slower? It's just going a tad bit too fast.

EDIT: I realize the speed is dependent on your browser size (ie if your browser width is smaller, it moves slower). Also the place where it cuts off is browser width dependent as well. If there's anyway to not make it dependent on that I think we would be good

@jdliaw
Copy link
Contributor Author

jdliaw commented Feb 20, 2018

should we just not show it on mobile view? since the bar is kinda too small

@binerys
Copy link
Member

binerys commented Feb 20, 2018

@jdliaw Yeah I agree. It's really small and would be impossible to read the phone number.

import isMobile from './misc/isMobile.js';

// styling
require('./StreamBar.scss');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not import? Keep it consistent

@@ -174,7 +182,7 @@ const RecentlyPlayed = React.createClass({
}
},
truncateName(name, l) {
return name.length > l ? `${name.substr(0, l - 2)}\u2026` : name;
return name.length > l ? name.substr(0, l - 2) + '\u2026' : name;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not string interpolation? It's the cool new thing >.>

<a href={track.url} target="_blank">
{track.artist}
</a>
{this.state.recentTracks.map(function(track, i) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arrow functions are also the cool new thing


import React from 'react';

require('./StreamMarquee.scss');
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as before, prefer import

@prop message: message to display on the marquee
*/

var StreamMarquee = React.createClass({
Copy link
Collaborator

Choose a reason for hiding this comment

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

use let or better yet const

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.

4 participants