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.

Sign up for GitHub

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

Jump to bottom

fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactoring formatTrackData #206

Merged
merged 8 commits into from
May 25, 2022
Merged

fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactoring formatTrackData #206

merged 8 commits into from
May 25, 2022

Conversation

tbradbury
Copy link
Contributor

@tbradbury tbradbury commented May 20, 2022 •
edited
Loading

PPDSC-2047

What

  1. When user moves the seekbar curser manually multiple disparate buffered sections can exist. However these seem to switch to the mirror opposite to what there where when you stop moving the curser.
  2. It was agreed to make it act more like the buffer on Youtube. You now only see the first buffer closet to the curser.
  3. You should not see multiple buffers. Buffer will be by the curser though its size depends on what is returned by the audio elements buffer property (so if can fluctuate in size).

Screenshot 2022-05-23 at 1 31 33 pm

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@github-actions github-actions bot added the fix This change fixes a bug label May 20, 2022
@newskit-engineering
Copy link
Collaborator

You can preview these changes on:
Documentation Site: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2047-seekbar-gaps
Storybook: http://ncu-newskit-docs.s3-website-eu-west-1.amazonaws.com/ppdsc-2047-seekbar-gaps/storybook

@Vanals Vanals added the ready for review Please assist in getting this reviewed label May 23, 2022
mstuartf
mstuartf reviewed May 23, 2022
View reviewed changes
src/audio-player-composable/components/seek-bar/utils.ts Outdated Show resolved Hide resolved
src/audio-player-composable/components/seek-bar/utils.ts Outdated
@@ -11,26 +11,47 @@ export const formatTrackData = (

const time = timeArr[0];
const bufferPositions = [];
const closestBuffer = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this should just be a single value not an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take a look

Copy link
Contributor Author

@tbradbury tbradbury May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm adding a single value to an array because it enable me to use the spread operator to add the correct value to the colors array (line 48) and the values array (line 54). If the array is empty I don't need to add a ternary operator to colors or values to ensure they have the right content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you. I'd prefer using a ternary operator later on but I don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've changed it

mstuartf
mstuartf previously approved these changes May 24, 2022
View reviewed changes
Vanals
Vanals reviewed May 24, 2022
View reviewed changes
src/audio-player-composable/__tests__/utils.test.tsx Outdated
// This can occur when a user moves ahead then back again, multiple disparate buffered sections can exist.
// Example here is buffered sections exist between 0-125 and 216-228. The current play position is at 32.
// It has been diecided to only show the first buffer section similar to how youtube does it.
Copy link
Contributor

Choose a reason for hiding this comment

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

lil typo diecided

Copy link
Contributor Author

@tbradbury tbradbury May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🤯 I'll sort 👍

@tbradbury tbradbury changed the title fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactor formatTrackData fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactoring formatTrackData May 24, 2022
@tbradbury tbradbury merged commit 1990f3f into main May 25, 2022
@tbradbury tbradbury deleted the fix/PPDSC-2047-seekbar-gaps branch May 25, 2022 09:06
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
@tbradbury
fix(PPDSC-2047): Resolve gaps in buffering seekbar by refactoring for…
2f093a5 
…matTrackData (#206)

* fix(PPDSC-2047): refactor formatTrackData

* fix(PPDSC-2047): refactor to send closest buffer

* fix(PPDSC-2047): add type to buffer array

* fix(PPDSC-2047): refactor closestBuffer

* fix(PPDSC-2047): fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstuartf mstuartf mstuartf approved these changes

@Vanals Vanals Vanals approved these changes

@jps jps Awaiting requested review from jps

@Xin00163 Xin00163 Awaiting requested review from Xin00163

@pdimova pdimova Awaiting requested review from pdimova

@baburay23 baburay23 Awaiting requested review from baburay23

@mutebg mutebg Awaiting requested review from mutebg

@evgenitsn evgenitsn Awaiting requested review from evgenitsn

@Srinivasan-Ramasamy Srinivasan-Ramasamy Awaiting requested review from Srinivasan-Ramasamy

Assignees
No one assigned
Labels
fix This change fixes a bug ready for review Please assist in getting this reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tbradbury @newskit-engineering @Vanals @mstuartf