-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: add range CSS parts #990
Conversation
@luwes is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
==========================================
- Coverage 78.55% 76.90% -1.65%
==========================================
Files 59 50 -9
Lines 11080 12095 +1015
Branches 0 699 +699
==========================================
+ Hits 8704 9302 +598
- Misses 2376 2768 +392
- Partials 0 25 +25 ☔ View full report in Codecov by Sentry. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9a32161
to
b1eee74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
src/js/media-chrome-range.ts
Outdated
@@ -240,13 +240,13 @@ template.innerHTML = /*html*/ ` | |||
<div id="startpoint"></div> | |||
<div id="endpoint"></div> | |||
<div id="appearance" part="appearance"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is appearance
an undocumented part? Or should it not be exposed? It's kind of an odd concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree now but it was documented unfortunately so I don't think we can remove it :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we don't frequently do this, but I think this is a good case to deprecate (and document the deprecation). I think several of these names may be confusing or at least not intuitive. The track-specific parts are not obviously track specific. I'm not sure why the div indicating what's been buffered is called "highlight"
? I don't have a clear sense of what "pointer"
actually is from the name or the description? Might be worth some time/thought/discussion getting these names right, since, per @luwes's last comment, once we add these parts + names it's a bigger deal to change or remove them.
If we do deprecate "appearance"
and decide to rename it in the deprecation process, I think we can just:
- add the new part name to the attribute
- update the docs indicating that appearance is deprecated and will be removed in a future release and they should use whatever the new part name is (which should also be documented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good yes, I don't want to rush this.
There's much to think about also heading in the future CSS vars vs CSS parts. And if we ever want to deprecate CSS vars in favor of a CSS part.
highlight
is called like that because it's in the base class MediaChromeRange.
It doesn't really know about buffering or even if it's a time range.
the appearance naming is from CSS and webkit uses this as well. it's the visual UI, the appearance :)
f72fab4
to
d6adb7c
Compare
add background and pointer parts and JS docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
fix #921, fix #983
thumb
,track
,progress
,buffered
.--media-range-track-color
CSS var is removed from JS docs, this was left over from the old structure with CSS linear-gradient where it represented the color of the range section on the right of the thumb.