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

Music: use sounds panel revision #58655

Merged
merged 5 commits into from
May 17, 2024
Merged

Conversation

breville
Copy link
Member

@breville breville commented May 15, 2024

With this change, the revised sounds panel introduced in #58275 becomes the default.

Two UI issues have been fixed:

  • When the current sound was the default, and the sounds panel was opened, and another sound was clicked, we instantly scrolled to that new sound.
  • When the current sound was not the default, and the sounds panel was opened, and the default sound was clicked, we did an eased scroll to the default sound.

The ScrollIntoView component has been renamed EaseIntoView to alleviate confusion with the browser's existing scrollIntoView function.

A DCDO flag of "music-lab-use-sounds-panel-1" can be set to true or the sounds-panel-2=false URL parameter can continue to be used to switch back to the legacy sounds panel.

@breville breville requested review from moneppo, a team, amy-b and markabarrett May 15, 2024 21:27
@breville breville changed the title Music: fix scrolling in sounds panel revision Music: use sounds panel revision May 15, 2024
Copy link
Contributor

@thomasoniii thomasoniii left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

// Capture whether the initially-selected sound is the default sound, in which
// case we will do a smooth scroll into view, rather than scroll directly
// to the current sound.
const isDefaultSoundSelected = useRef(
Copy link
Contributor

Choose a reason for hiding this comment

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

To check my understanding: we're using useRef so that the below useEffect only runs on initial render, and not whenever currentValue changes, right? If so, mind updating the comment to capture that?

Copy link
Member Author

@breville breville May 16, 2024

Choose a reason for hiding this comment

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

Yes, that's right. Comment updated.

@breville breville merged commit a9a78cc into staging May 17, 2024
2 checks passed
@breville breville deleted the music-sounds-panel-fun-fix-scrolling branch May 17, 2024 07:02
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.

None yet

3 participants