Prospective solution for livestream appearance #1077

jreiser1 · 2021-10-17T21:00:48Z

Fix partially implemented for #970

njs 12.14.1

Merge with main project master branch

Travis Travis Build build Windows Tests No cache npm install Linux njs 12.14.1 build build travis windows build linux test export export node windows travis_wait linux tests javascript sudo script local_dir commit commit

nukeop · 2021-10-17T21:34:24Z

packages/ui/lib/utils.ts

-  if (!_.isFinite(parseInt(duration)) || duration <= 0) {
+export function formatDuration(duration, livestream?) {
+  if (livestream) {
+    return 'LIVE';


This string should be localized

fyi, in UI package localized string must be passed from higher component, can not require useTranslation

Understood, I'll add localization. @haidang666 Should localization happen in the Seekbar component?

Yes, but there is one more thing, you don't need to add the translation key to all languages JSON files, only in en.json.

Ok, I've got the Seekbar component in the app package modified as follows:

However this doesn't seem to have affected the actual timestamps when a song is playing since those are calculated and displayed dynamically (as far as I can tell). Is there just something I'm missing for how to implement the localization properly?

I think you got the wrong SeekBar. The app/PlayerBarContainer uses ui/PlayerBar which uses the SeekBar in UI package (https://github.com/nukeop/nuclear/blob/master/packages/ui/lib/components/Seekbar/index.tsx)

Ok, I think I got the localization working this time. I added the translation to the PlayerBarContainer like you suggested and re-worked some of our changes in the Seekbar UI component

one more thing u should revert other language file change, only need keep the english one

I've reverted the other language file changes. Only en.json will reflect the live keys.

Pulling in localization from Livestreamfix2

Adding check for NaN timeToEnd to fix seekbar UI bug

adding localization to live timestamps

nukeop · 2021-10-21T20:51:58Z

packages/ui/lib/components/PlayerBar/index.tsx

@@ -23,6 +23,9 @@ export type PlayerBarProps = PlayerControlsProps &
  };

 const VOLUME_POPUP_BREAKPOINT = 570;
+
+let livestream = false;


can you rename it to isLivestream for consistency of naming with other boolean variables?

This should be reflected in latest commit.

Thanks!

nukeop · 2021-10-21T20:57:39Z

packages/ui/lib/components/PlayerBar/index.tsx

+  }
+  if (typeof timeToEnd === 'string') {
+    livestream = true;
+    fill = 100;


You should never mutate props. Instead of doing all this, you should pass different props to child elements. You can also determine whether we're dealing with a live stream or not in a single line.

E.g.:

const isLivestream = isNan(timetoEnd) || isString(timeToEnd);

Then depending on this variable, pass a different fill value to the seekbar:

fill={isLivestream ? 100 : fill}

const isLivestream = isNan(timetoEnd) || isString(timeToEnd);

Almost have something like this working, above line fills SeekBar by default but the line in our repo now has it empty by default.

fill={isLivestream ? 100 : fill}

I like this line, I've added it to the PR.

Thanks for reviewing!

nukeop · 2021-10-21T20:59:15Z

packages/ui/lib/components/QueueItem/index.jsx

@@ -62,7 +62,7 @@ export const QueueItem = ({

        <div className={styles.item_duration_container}>
          <div className={styles.item_duration}>
-            {duration}
+            {duration === '00:00' && !isLoading ? 'LIVE' : duration}


Can you use the same localization method for this 'LIVE' string as well? Generally we don't want to have any bare strings that aren't localized, with some common sense exceptions (like this 00:00 here).

Indeed this file does not need to be changed at all, I've reverted this back to its previous state as the localization for this component is already done in app/components/PlayQueue/index.js

nukeop · 2021-10-21T21:00:52Z

Thanks for iterating on this with us. I added some comments to your changes. There are some typing errors in the CI logs, could you please take a look at them? I can fix them for you later if you don't want to do it too.

…s in components/PlayQueue)

…ream localizations.

1lluzi0nzz · 2021-10-28T17:12:08Z

Type Errors should be gone now.

Thanks!

nukeop · 2021-11-08T20:16:20Z

Thanks! I squashed and merged this now

Jacob Netwal and others added 30 commits September 6, 2021 14:43

Travis

2bd857c

Travis

dc9e5d3

Build

ca7e4d0

build

10d557f

Windows

e69e606

Tests

2795303

No cache

524bdf6

npm install

0b90ba2

Linux

c0d03df

njs 12.14.1

build

cf41bc2

build

11d8e31

travis

0aa17c1

windows

0c55278

build

50dcb22

linux

dd89746

test

9d47b76

export

9a356ec

export

9b1b6b8

node

2980902

windows

66591c5

travis_wait linux

a8a4fc1

tests

42ff4f3

javascript

73abe93

sudo

466faae

script

61fca26

local_dir

7f01d81

commit

7a9a14d

commit

88aaf8f

Merge pull request #3 from nukeop/master

271006f

Merge with main project master branch

Combining all Travis setup commits

3afc999

Travis Travis Build build Windows Tests No cache npm install Linux njs 12.14.1 build build travis windows build linux test export export node windows travis_wait linux tests javascript sudo script local_dir commit commit

nukeop reviewed Oct 17, 2021

View reviewed changes

nukeop added needs changes The author needs to make changes to this pull request. under review This pull request is being reviewed by maintainers. labels Oct 18, 2021

Jacob Netwal and others added 7 commits October 18, 2021 20:20

Livestream bottom right -'LIVE' > 'LIVE'

c4fe0a7

adding item for "Live" timestamp to language files

dba5177

removing unnecessary logging

8d045db

Merge pull request #15 from MSU-CS4360-Nuclear/livestreamfix2

091d160

Pulling in localization from Livestreamfix2

QueueItem duration is localized with livestreams ie Live, En Vivo, etc

1d313f0

Adding check for NaN timeToEnd to fix seekbar UI bug

3706cd0

Merge pull request #16 from MSU-CS4360-Nuclear/livestreamfix2

78136ba

Adding check for NaN timeToEnd to fix seekbar UI bug

haidang666 linked an issue Oct 21, 2021 that may be closed by this pull request

Improve appearance when playing livestream #970

Closed

jreiser1 and others added 3 commits October 21, 2021 00:13

adding localization to live timestamps

569828e

adding localization to live timestamps

07b413f

adding localization to live timestamps

Removing live keys from all language files except en.json

47aa7d7

nukeop reviewed Oct 21, 2021

View reviewed changes

Jacob Netwal and others added 9 commits October 21, 2021 15:15

livestream > isLivestream, Reverted change in QueueItem.tsx (change i…

7b02ff6

…s in components/PlayQueue)

1 Line for isLivestream & fill

fc6023f

hooks.ts type errors

3d260f5

Seekbar Type Errors

5b5a954

Merge branch 'nukeop:master' into master

8ed3641

Seekbar types

2a6d834

formatDuration test for string 'test' should now be 'test' for Livest…

5412342

…ream localizations.

Fixed Snapshot tests

ff2f0dd

revert album_year test

67c22eb

nukeop merged commit 4487fe3 into nukeop:master Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prospective solution for livestream appearance #1077

Prospective solution for livestream appearance #1077

jreiser1 commented Oct 17, 2021 •

edited by haidang666

Loading

nukeop Oct 17, 2021

haidang666 Oct 18, 2021

jreiser1 Oct 19, 2021 •

edited

Loading

haidang666 Oct 20, 2021

jreiser1 Oct 21, 2021

haidang666 Oct 21, 2021

jreiser1 Oct 21, 2021

haidang666 Oct 21, 2021

1lluzi0nzz Oct 21, 2021

nukeop Oct 21, 2021

1lluzi0nzz Oct 21, 2021

nukeop Oct 21, 2021

1lluzi0nzz Oct 21, 2021

nukeop Oct 21, 2021

1lluzi0nzz Oct 21, 2021

nukeop commented Oct 21, 2021

1lluzi0nzz commented Oct 28, 2021

nukeop commented Nov 8, 2021

Prospective solution for livestream appearance #1077

Prospective solution for livestream appearance #1077

Conversation

jreiser1 commented Oct 17, 2021 • edited by haidang666 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreiser1 Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nukeop commented Oct 21, 2021

1lluzi0nzz commented Oct 28, 2021

nukeop commented Nov 8, 2021

jreiser1 commented Oct 17, 2021 •

edited by haidang666

Loading

jreiser1 Oct 19, 2021 •

edited

Loading