fix(audio-player): use rh-icon instead of custom svg for scrubbers#2095
fix(audio-player): use rh-icon instead of custom svg for scrubbers#2095
Conversation
🦋 Changeset detectedLatest commit: 8c8f001 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: -450 B (-0.22%) Total Size: 206 kB
ℹ️ View Unchanged
|
bennypowers
left a comment
bennypowers
left a comment
There was a problem hiding this comment.
I'm all for it, from a code perspective, but seeking approval from our betters in design @marionnegp @coreyvickery
There was a problem hiding this comment.
The scrubber icon size looks good to me!
- Can the disabled scrubber buttons be
--rh-color-gray-30(light theme) and--rh-color-gray-50(dark theme) for now? (We'll eventually add disabled color tokens.)
The rest of these can be updated in a separate PR if you'd prefer:
-
Speed controls don't seem to be using the right color in dark theme.
-
Play button in light theme player should be in a
darkestcircle with a white play icon:
-
Play button in dark theme player should be in a white circle with a
darkestplay icon:
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> |
|
@bennypowers I can't believe the resolution was as simple as removing the |
@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. |
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? |
|
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
@marionnegp this feature set is not outlined in the Figma file. Guidance here on what this should be is appreciated. |
It was in |
The docs show images of the highlight for light theme, and that blue looks most like 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? |
I believe that is the case, yes. |
|
Noting the auto scroll download toolbar is now missing from demos on ux.redhat.com Should look like: Now looks like: |
|
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. |
|
Close in favor of: #2469 |
github-project-automation
Bot
moved this from Review 🔍
to Done ☑️
in Red Hat Design System
4 participants
What I did
rh-iconfor scrubbersCloses #2049
TODO:
Testing Instructions
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.