red-hat-design-system icon indicating copy to clipboard operation
red-hat-design-system copied to clipboard

fix(audio-player): use rh-icon instead of custom svg for scrubbers

Open zeroedin opened this issue 11 months ago • 11 comments

What I did

  1. Replaced custom svg's with rh-icon for scrubbers

Closes #2049

TODO:

  • [ ] Docs images as listed in issue

Testing Instructions

  1. View deploy preview

Notes to Reviewers

@marionnegp I added the rh-icon with the same size as the play button (24px) as that looked closest to the previous state. If it should be a different size let me know. I'll leave PR in draft and if you want to make the changes to the docs images on this same branch go right ahead.

zeroedin avatar Dec 16 '24 19:12 zeroedin

🦋 Changeset detected

Latest commit: 8c8f0014ce81e0b16841ebea9bfd0f91c74bbb7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@rhds/elements Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Dec 16 '24 19:12 changeset-bot[bot]

Deploy Preview for red-hat-design-system ready!

Name Link
Latest commit 8c8f0014ce81e0b16841ebea9bfd0f91c74bbb7a
Latest deploy log https://app.netlify.com/sites/red-hat-design-system/deploys/67a873610b7d170008288d21
Deploy Preview https://deploy-preview-2095--red-hat-design-system.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 16 '24 19:12 netlify[bot]

Size Change: -450 B (-0.22%)

Total Size: 206 kB

Filename Size Change
./elements.js 473 B -3 B (-0.63%)
./elements/rh-audio-player/rh-audio-player.js 12.6 kB -447 B (-3.41%)
ℹ️ View Unchanged
Filename Size
./elements/rh-accordion/context.js 162 B
./elements/rh-accordion/rh-accordion-header.js 2.69 kB
./elements/rh-accordion/rh-accordion-panel.js 1.35 kB
./elements/rh-accordion/rh-accordion.js 3.21 kB
./elements/rh-alert/rh-alert.js 4.26 kB
./elements/rh-audio-player/rh-audio-player-about.js 1.8 kB
./elements/rh-audio-player/rh-audio-player-rate-stepper.js 1.85 kB
./elements/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 1.53 kB
./elements/rh-audio-player/rh-audio-player-subscribe.js 1.38 kB
./elements/rh-audio-player/rh-cue.js 1.95 kB
./elements/rh-audio-player/rh-transcript.js 2.7 kB
./elements/rh-avatar/random-pattern-controller.js 2.72 kB
./elements/rh-avatar/rh-avatar.js 2.86 kB
./elements/rh-back-to-top/rh-back-to-top.js 2.1 kB
./elements/rh-badge/rh-badge.js 1.55 kB
./elements/rh-blockquote/rh-blockquote.js 1.36 kB
./elements/rh-breadcrumb/rh-breadcrumb.js 1.5 kB
./elements/rh-button/rh-button.js 4.24 kB
./elements/rh-card/rh-card.js 3.58 kB
./elements/rh-code-block/prism.css.js 667 B
./elements/rh-code-block/prism.js 572 B
./elements/rh-code-block/rh-code-block.js 7.34 kB
./elements/rh-cta/rh-cta.js 4.04 kB
./elements/rh-dialog/rh-dialog.js 4.78 kB
./elements/rh-dialog/yt-api.js 617 B
./elements/rh-footer/rh-footer-block.js 714 B
./elements/rh-footer/rh-footer-copyright.js 362 B
./elements/rh-footer/rh-footer-links.js 1.17 kB
./elements/rh-footer/rh-footer-social-link.js 964 B
./elements/rh-footer/rh-footer-universal.js 3.99 kB
./elements/rh-footer/rh-footer.js 4.96 kB
./elements/rh-health-index/rh-health-index.js 2.35 kB
./elements/rh-icon/rh-icon.js 2.47 kB
./elements/rh-icon/ssr.js 181 B
./elements/rh-menu/rh-menu.js 1.29 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 2.47 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 1.3 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-menu.js 1.75 kB
./elements/rh-navigation-secondary/rh-navigation-secondary-overlay.js 571 B
./elements/rh-navigation-secondary/rh-navigation-secondary.js 5.26 kB
./elements/rh-navigation-secondary/test/fixtures.js 769 B
./elements/rh-pagination/rh-pagination.js 5.4 kB
./elements/rh-site-status/rh-site-status.js 2.5 kB
./elements/rh-skip-link/rh-skip-link.js 1.19 kB
./elements/rh-spinner/rh-spinner.js 1.38 kB
./elements/rh-stat/rh-stat.js 2.13 kB
./elements/rh-subnav/rh-subnav.js 2.73 kB
./elements/rh-surface/rh-surface.js 1.14 kB
./elements/rh-surface/test/elements.js 423 B
./elements/rh-switch/rh-switch.js 2.93 kB
./elements/rh-table/rh-sort-button.js 1.49 kB
./elements/rh-table/rh-table.js 3.48 kB
./elements/rh-tabs/context.js 160 B
./elements/rh-tabs/rh-tab-panel.js 1.04 kB
./elements/rh-tabs/rh-tab.js 3.02 kB
./elements/rh-tabs/rh-tabs.js 3.77 kB
./elements/rh-tag/rh-tag.js 2.79 kB
./elements/rh-tile/rh-tile-group.js 1.81 kB
./elements/rh-tile/rh-tile.js 5.09 kB
./elements/rh-timestamp/rh-timestamp.js 983 B
./elements/rh-tooltip/rh-tooltip.js 2.73 kB
./elements/rh-video-embed/rh-video-embed.js 4.59 kB
./lib/context/color/consumer.js 1.41 kB
./lib/context/color/controller.js 947 B
./lib/context/color/provider.js 2.18 kB
./lib/context/event.js 593 B
./lib/context/headings/consumer.js 722 B
./lib/context/headings/controller.js 1.12 kB
./lib/context/headings/provider.js 1.24 kB
./lib/DirController.js 565 B
./lib/elements/rh-context-demo/rh-context-demo.js 1.28 kB
./lib/elements/rh-context-picker/rh-context-picker.js 2.24 kB
./lib/environment.js 194 B
./lib/functions.js 175 B
./lib/I18nController.js 1.38 kB
./lib/ScreenSizeController.js 849 B
./lib/ssr-controller.js 251 B
./react/rh-accordion/rh-accordion-header.js 199 B
./react/rh-accordion/rh-accordion-panel.js 185 B
./react/rh-accordion/rh-accordion.js 215 B
./react/rh-alert/rh-alert.js 184 B
./react/rh-audio-player/rh-audio-player-about.js 191 B
./react/rh-audio-player/rh-audio-player-rate-stepper.js 213 B
./react/rh-audio-player/rh-audio-player-scrolling-text-overflow.js 214 B
./react/rh-audio-player/rh-audio-player-subscribe.js 196 B
./react/rh-audio-player/rh-audio-player.js 183 B
./react/rh-audio-player/rh-cue.js 195 B
./react/rh-audio-player/rh-transcript.js 207 B
./react/rh-avatar/rh-avatar.js 173 B
./react/rh-back-to-top/rh-back-to-top.js 183 B
./react/rh-badge/rh-badge.js 174 B
./react/rh-blockquote/rh-blockquote.js 179 B
./react/rh-breadcrumb/rh-breadcrumb.js 179 B
./react/rh-button/rh-button.js 174 B
./react/rh-card/rh-card.js 172 B
./react/rh-code-block/rh-code-block.js 181 B
./react/rh-cta/rh-cta.js 170 B
./react/rh-dialog/rh-dialog.js 203 B
./react/rh-footer/rh-footer-block.js 184 B
./react/rh-footer/rh-footer-copyright.js 187 B
./react/rh-footer/rh-footer-links.js 185 B
./react/rh-footer/rh-footer-social-link.js 193 B
./react/rh-footer/rh-footer-universal.js 188 B
./react/rh-footer/rh-footer.js 174 B
./react/rh-health-index/rh-health-index.js 184 B
./react/rh-icon/rh-icon.js 202 B
./react/rh-menu/rh-menu.js 173 B
./react/rh-navigation-secondary/rh-navigation-secondary-dropdown.js 217 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu-section.js 205 B
./react/rh-navigation-secondary/rh-navigation-secondary-menu.js 199 B
./react/rh-navigation-secondary/rh-navigation-secondary-overlay.js 201 B
./react/rh-navigation-secondary/rh-navigation-secondary.js 213 B
./react/rh-pagination/rh-pagination.js 178 B
./react/rh-site-status/rh-site-status.js 181 B
./react/rh-skip-link/rh-skip-link.js 181 B
./react/rh-spinner/rh-spinner.js 175 B
./react/rh-stat/rh-stat.js 171 B
./react/rh-subnav/rh-subnav.js 175 B
./react/rh-surface/rh-surface.js 175 B
./react/rh-switch/rh-switch.js 185 B
./react/rh-table/rh-sort-button.js 213 B
./react/rh-table/rh-table.js 174 B
./react/rh-tabs/rh-tab-panel.js 181 B
./react/rh-tabs/rh-tab.js 187 B
./react/rh-tabs/rh-tabs.js 174 B
./react/rh-tag/rh-tag.js 182 B
./react/rh-tile/rh-tile-group.js 183 B
./react/rh-tile/rh-tile.js 194 B
./react/rh-timestamp/rh-timestamp.js 176 B
./react/rh-tooltip/rh-tooltip.js 175 B
./react/rh-video-embed/rh-video-embed.js 227 B
./uxdot/uxdot-best-practice.js 742 B
./uxdot/uxdot-copy-button.js 1.2 kB
./uxdot/uxdot-copy-permalink.js 1.1 kB
./uxdot/uxdot-example.js 1.17 kB
./uxdot/uxdot-feedback.js 727 B
./uxdot/uxdot-header.js 1.07 kB
./uxdot/uxdot-hero.js 679 B
./uxdot/uxdot-installation-tabs.js 675 B
./uxdot/uxdot-masthead.js 809 B
./uxdot/uxdot-pattern-ssr-controller-client.js 604 B
./uxdot/uxdot-pattern-ssr-controller-server.js 1.68 kB
./uxdot/uxdot-pattern-ssr-controller.js 213 B
./uxdot/uxdot-pattern.js 2.12 kB
./uxdot/uxdot-repo-status-checklist.js 1.16 kB
./uxdot/uxdot-repo-status-list.js 1.07 kB
./uxdot/uxdot-repo-status-table.js 782 B
./uxdot/uxdot-repo.js 1.17 kB
./uxdot/uxdot-search.js 2.39 kB
./uxdot/uxdot-sidenav.js 2.67 kB
./uxdot/uxdot-spacer-tokens-table.js 2.45 kB
./uxdot/uxdot-toc.js 1.13 kB

compressed-size-action

github-actions[bot] avatar Dec 16 '24 19:12 github-actions[bot]

  • Speed controls don't seem to be using the right color in dark theme.

This looks like it opens up a bigger issue on audio player. Will need to dig deeper tomorrow to see what is going on, Color context from the outside leaks into audio player and it has an internal surface which doesn't set to match the parent context:

Example of issue:

<rh-context-demo color-palette="darkest"> <!--sets context to dark-->
  <rh-audio-player> <!-- dark background is set -->
    <!-- shadowroot-->
       <rh-surface> <!-- internal context is set to light (note no color palette) -->
          Text / Buttons <!-- all think they are on a light background -->
        </rh-surface>
   </rh-audio-player>
 </rh-context-demo>

zeroedin avatar Dec 17 '24 21:12 zeroedin

@bennypowers I can't believe the resolution was as simple as removing the rh-surface from the shadow root. However, I couldn't find a reason for it. Can you double-check me here? I may be overlooking something.

zeroedin avatar Dec 18 '24 20:12 zeroedin

Play button in light theme player should be in a darkest circle with a white play icon:

@marionnegp, I'll look at these changes with the circle not being present on the play icon now. The circle was likely part of the custom SVG previously.

zeroedin avatar Dec 18 '24 20:12 zeroedin

The circle was likely part of the custom SVG previously.

This was not a true statement 😆 , that said I'm now adjusting the variables used directly based on the context.

#play,
#full-play {
  border-radius: 50%;
  background-color: var(--rh-color-surface-darkest, #151515);
  color:
    var(--rh-audio-player-icon-background-color,
      var(--rh-color-text-primary-on-dark, #ffffff));
}

.dark {
  #play,
  #full-play {
    background-color: var(--rh-color-surface-lightest, #ffffff);
    color:
      var(--rh-audio-player-icon-background-color,
        var(--rh-color-text-primary-on-light, #151515));
  }
}

@marionnegp I feel like the play icon may need micro-adjusting to make it feel centered (visually). I think we've talked about this before somewhere. Déjà vu?

zeroedin avatar Dec 18 '24 20:12 zeroedin

image

Another bug found, the highlight in the text being read in the transcript also appears to be off.

Does not appear to be new / introduced in this PR either, also found on ux.redhat.com

image

@marionnegp this feature set is not outlined in the Figma file. Guidance here on what this should be is appreciated.

zeroedin avatar Dec 18 '24 20:12 zeroedin

I think we've talked about this before somewhere. Déjà vu?

It was in rh-video-embed. it looks like the difference is we need to use an rh-button variant="play" which will get us the adjusted variant we are looking for. Looking to see if we can reuse that here.

zeroedin avatar Dec 19 '24 14:12 zeroedin

Another bug found, the highlight in the text being read in the transcript also appears to be off.

The docs show images of the highlight for light theme, and that blue looks most like --rh-color-blue-20. @coreyvickery, did you specify a highlight color for light and dark themes?

Looking at Nikki's original audio player PR and the transcript demo, it might have been using the default browser highlight. Maybe this was a bug introduced when we made changes to the theming?

marionnegp avatar Dec 19 '24 15:12 marionnegp

Maybe this was a bug introduced when we made changes to the theming?

I believe that is the case, yes.

zeroedin avatar Dec 19 '24 15:12 zeroedin

Noting the auto scroll download toolbar is now missing from demos on ux.redhat.com

Should look like: image

Now looks like: image

zeroedin avatar May 23 '25 19:05 zeroedin

Given changes in theming between the opening of this PR and its becoming stale, I recommend closing this PR in favor of opening a newly cut branch of the current state main to deal with this issue and reimplement the changes instead of dealing with the conflicts that are bound to happen here. Leaving it open till a new PR is cut that references this one.

zeroedin avatar May 23 '25 19:05 zeroedin

Close in favor of: https://github.com/RedHat-UX/red-hat-design-system/pull/2469

zeroedin avatar Jul 08 '25 17:07 zeroedin