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

HLS changes #81

Merged
merged 5 commits into from
Apr 21, 2024
Merged

HLS changes #81

merged 5 commits into from
Apr 21, 2024

Conversation

P4likka
Copy link
Contributor

@P4likka P4likka commented Apr 19, 2024

No description provided.

@P4likka P4likka force-pushed the hls branch 3 times, most recently from 093b539 to 0a0786a Compare April 19, 2024 13:41
pages/_app.tsx Outdated

useEffect(() => {
loadAudioStream();
}, [loadAudioStream]);
Copy link
Contributor

Choose a reason for hiding this comment

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

loadAudioStream saa uuden viittauksen jokaisella renderillä, joten useEffectiä kutsutaan koko ajan ja hommat alustellaan uudestaan. Siirrä loadAudioStream useEffectin sisälle ja kutsu siellä kerran 👌🏼

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah katos sitä tarvitaankin tuolla alempana 🤔 Siinä tapauksesssa useCallback vaan ympärille

@@ -89,7 +89,7 @@ export const parseSheetToShowList = async (
const isNight = color === Color.Night || getIsNightTime(startDate);
const showColor = isNight
? Color.Night
: (color === Color.Promote || color === Color.Editorial)
: color === Color.Promote || color === Color.Editorial
Copy link
Contributor

Choose a reason for hiding this comment

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

Testaamatta sanoisin, et sulut tarvitaan 🤔 ternaryt on vähän hassuja muiden ehtojen kanssa, joten kannattaa vielä kattoo, et tuleehan kaikki oikein 👍

Copy link
Member

Choose a reason for hiding this comment

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

Prettier oli tätä mieltä

Copy link
Contributor

@niemisami niemisami left a comment

Choose a reason for hiding this comment

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

useEffect kaipaa lempeä, mut tosi kiva nähä tää toiminnassa 💪🏼

const handlePlayPause = () => {
setPlayClicked(true);

if (audioEl.current.paused) audioEl.current.play();
else {
// Pause, but then load the stream again ready to start
audioEl.current.pause();
audioEl.current.src = AUDIO_STREAM_URL;
loadAudioStream();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm lataakos tää mahdollisesti väärän chunkin muistiin? Jos pauseen, odottelen tunnin, pistän päälle nii osaako jotenkin hakee sen uusimman vai halutaanko nimenomaan, et jatkaa vanhasta kohdasta?

Vanhassa playerissä tuli itelle vastaan joskus tää sama, et vanha kohta jäi muistiin (which is nice), mutta hetken päästä se kuitenkin hyppäs nykyhetkeen. Miten mahtaa toimia ja halutaan toimivan?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, hyvä huomio. Oiskohan tää parempi jos loadAudioStream kutsuttaisiin play:n painamisessa, eikä pausen? Voisin testata tän.

pages/_app.tsx Outdated
<audio ref={audioEl}>
<source src={AUDIO_STREAM_URL} type="audio/mpeg" />
</audio>
<audio ref={audioEl}></audio>
Copy link
Contributor

Choose a reason for hiding this comment

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

on positettu, niin eikö sitä tarvita enää, jos hlssää ei tueta? Loadissa on fallback vanhaan mediaan, mut pitäiskö tässäkin ottaa vanha tapa vielä käyttöön siinä tapauksessa?

Copy link
Contributor

@niemisami niemisami left a comment

Choose a reason for hiding this comment

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

Jos vain toi play/pausen toiminnallisuus on toimiva ja haluttu ominaisuus 👌🏼👌🏼

@kovipu kovipu merged commit fcec9ff into develop Apr 21, 2024
2 checks passed
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.

3 participants