feat: Migrate all attributes to lowercase ('smushedcase'). #537

cjpillsbury · 2023-04-17T21:50:09Z

Migrates all well-defined attributes in Media Chrome elements from kebab-case to lowercase (aka "smushedcase").

vercel · 2023-04-17T21:50:14Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
media-chrome	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 18, 2023 7:51pm
media-chrome-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 18, 2023 7:51pm

cjpillsbury · 2023-04-17T21:51:32Z

examples/nextjs-with-typescript/pages/index.tsx

@@ -1,31 +1,16 @@
 import type { NextPage } from 'next';


NOTE: Need to revert this. Hold over from presentation testing.

cjpillsbury · 2023-04-17T21:54:32Z

src/js/index.d.ts

@@ -0,0 +1,200 @@
+export * as constants from "./constants.js";


NOTE: Going to drop this in this PR, but this was the quick fix solution to get all of the TypeScript types in place for React. We should instead just completely revamp how the React wrappers work in our v1.0 effort.

cjpillsbury · 2023-04-17T21:55:22Z

src/js/media-chrome-button.js

@@ -129,14 +129,14 @@ class MediaChromeButton extends window.HTMLElement {
  enable() {
    this.addEventListener('click', this.#clickListener);
    this.addEventListener('keydown', this.#keydownListener);
-    this.setAttribute('tabindex', '0');
+    this.tabIndex = 0;


NOTE: Moving to using the tabIndex prop instead for forward-looking dreams.

cjpillsbury · 2023-04-17T21:58:48Z

src/js/media-container.js

+       * if gestures are disabled, don't accept pointer-events
+       */ ''
+    }
+    :host(:not([${Attributes.AUDIO}])[${


oof formatter really did a number on this one. Will unmangle.

can this be formatted similar to how it was?

which formatter did this? yarn format?

I think it was some default prettier line length creeping in? Haven't investigated.

cjpillsbury · 2023-04-17T22:00:13Z

src/js/media-container.js

+    }
+    :host([${Attributes.USER_INACTIVE}]:not([${
+  MediaUIAttributes.MEDIA_PAUSED
+}]):not([${MediaUIAttributes.MEDIA_IS_CASTING}]):not([${


same for this uggo.

cjpillsbury · 2023-04-17T22:04:57Z

src/js/media-time-display.js

@@ -12,8 +19,8 @@ const ButtonPressedKeys = ['Enter', ' '];
 const DEFAULT_TIMES_SEP = '&nbsp;/&nbsp;';

 const formatTimesLabel = (el, { timesSep = DEFAULT_TIMES_SEP } = {}) => {
-  const showRemaining = el.getAttribute('remaining') != null;
-  const showDuration = el.getAttribute('show-duration') != null;
+  const showRemaining = el.showRemaining;


NOTE: This feels like scope creep for this round. Hoping for a gut check from others if you think I should drop it (still around in other branches).

…wercase).

cjpillsbury · 2023-04-17T22:24:35Z

src/js/themes/microvideo.js

@@ -102,7 +102,7 @@ template.innerHTML = /*html*/`
    align-items: start;
  }

-  :host([control-bar-place$="end"]) media-control-bar {
+  :host([controlbarplace$="end"]) media-control-bar {


NOTE: Should discuss - normalize so official attrs for official themes are also lowercase/"smushedcase" (See below for at least one particular friction point/tradeoff).

cjpillsbury · 2023-04-17T22:25:33Z

src/js/themes/microvideo.js

@@ -440,7 +440,7 @@ template.innerHTML = /*html*/`
 </template>

 <media-controller
-  style="--_control-bar-place-self:{{controlBarPlace ?? 'unset'}}"
+  style="--_control-bar-place-self:{{controlbarplace ?? 'unset'}}"


NOTE: Because we use JS to translate attr -> prop, using all lowercase/"smushedcase" here doesn't allow us to use camelCase to translate (since there are no delimiters to match against with the new names).

We may want to treat this as a separate effort and descope it for this initial round of changes.

luwes · 2023-04-17T22:27:25Z

src/js/media-playback-rate-button.js

    ];
  }

+  /** @type number[] | undefined */
+  #_rates;


can we leave the underscore, use #rates instead?

don't really see a reason for an added underscore.

Yup we can def drop the underscore. ❤️

luwes · 2023-04-17T22:31:12Z

src/js/media-time-display.js

+  /** @type boolean | undefined */
+  #_showDuration;
+  /** @type boolean | undefined */
+  #_remaining;


here too the underscores

Will either remove or rename before merging, depending on team decision here.

gkatsev · 2023-04-17T22:33:11Z

src/js/media-playback-rate-button.js

@@ -60,12 +63,22 @@ class MediaPlaybackRateButton extends MediaChromeButton {
    super.attributeChangedCallback(attrName, oldValue, newValue);
  }

+  get rates() {


this seems like scope-creep.
This getter should return a DOMTokenList, while the setter should accept an array.

Cool I agree let's just drop all of the property changes in this PR. @luwes when we re-add we'll be sure to account for your private variable naming convention prefs.

codecov · 2023-04-17T23:04:17Z

Codecov Report

Merging #537 (c0ffeb6) into main (8671210) will decrease coverage by 0.03%.
The diff coverage is 77.47%.

@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   76.41%   76.38%   -0.03%     
==========================================
  Files          43       43              
  Lines        7056     7141      +85     
==========================================
+ Hits         5392     5455      +63     
- Misses       1664     1686      +22

Impacted Files	Coverage Δ
src/js/media-playback-rate-button.js	`33.72% <40.00%> (-12.63%)`	⬇️
src/js/media-seek-backward-button.js	`52.22% <41.66%> (+2.22%)`	⬆️
src/js/media-seek-forward-button.js	`54.25% <46.15%> (+2.03%)`	⬆️
src/js/controller.js	`64.46% <50.00%> (ø)`
src/js/media-poster-image.js	`62.96% <60.00%> (+2.43%)`	⬆️
src/js/media-container.js	`80.19% <78.65%> (-0.35%)`	⬇️
src/js/media-captions-button.js	`53.48% <80.00%> (+1.87%)`	⬆️
src/js/media-controller.js	`75.07% <80.95%> (+0.37%)`	⬆️
src/js/media-loading-indicator.js	`89.24% <83.33%> (+0.27%)`	⬆️
src/js/media-time-display.js	`78.10% <85.18%> (+1.26%)`	⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…on build.

cjpillsbury · 2023-04-18T14:13:16Z

package.json

@@ -25,7 +25,7 @@
    "build:iife": "esbuild src/js/index.js --bundle --target=es2019 --format=iife --outdir=dist/iife --minify --sourcemap --global-name=MediaChrome && yarn size",
    "build:react": "node ./scripts/react/build.js",
    "build:manifest": "npx @custom-elements-manifest/analyzer analyze --config scripts/custom-elements-manifest.config.js",
-    "build": "yarn build:esm && yarn build:cjs && yarn build:iife && yarn build:react && yarn build:types && yarn build:manifest",
+    "build": "yarn build:types && yarn build:esm && yarn build:cjs && yarn build:iife && yarn build:react && yarn build:manifest",


NOTE: switch the order to ensure types are available before build (for dev watch behavior/expectations)

luwes

pre-approving, just need to fix the CSS formatting a little bit

gkatsev · 2023-04-18T16:47:59Z

src/js/media-playback-rate-button.js

+  // NOTE: Adding for TypeScript Errors. Followup should add correct getter/setter & private var (CJP)
+  /** @type number[] | undefined */
+  _rates;


is this used anywhere?

gkatsev · 2023-04-18T16:50:16Z

src/js/media-poster-image.js

@@ -35,7 +40,7 @@ const setBackgroundImage = (el, image) => {

 class MediaPosterImage extends window.HTMLElement {
  static get observedAttributes() {
-    return ['placeholder-src', 'src'];
+    return [Attributes.PLACEHOLDER_SRC, Attributes.SRC];


non-blocker: alternative is to do Object.values(Attributes)

decided to not use values just because that's "incidental" and not "definitional" (aka all of the values in Attributes happen to be observedAttributes, but that could change and isn't inherent to their relationship).

Ah, right. Worked here incidentally. Wasn't a blocker or necessary anyway.

… formatting.

feat: Migrate all attributes to lowercase ('smushedcase').

e47e109

vercel bot deployed to Preview – media-chrome April 17, 2023 21:50 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 21:50 View deployment

cjpillsbury commented Apr 17, 2023

View reviewed changes

fix: media-time-display wrong setter value.

87da021

vercel bot deployed to Preview – media-chrome April 17, 2023 22:08 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 22:08 View deployment

fix: typo in microvideo attribute name.

53c118f

vercel bot deployed to Preview – media-chrome April 17, 2023 22:14 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 22:14 View deployment

fix: Clean up microvideo + example (discuss: camelcase + kebab vs. lo…

ec84b49

…wercase).

vercel bot deployed to Preview – media-chrome April 17, 2023 22:22 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 22:22 View deployment

cjpillsbury commented Apr 17, 2023

View reviewed changes

luwes reviewed Apr 17, 2023

View reviewed changes

gkatsev reviewed Apr 17, 2023

View reviewed changes

test: Update tests to reflect new attribute names.

904bbc6

vercel bot deployed to Preview – media-chrome April 17, 2023 22:58 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 22:58 View deployment

revert nextjs changes.

554933a

vercel bot deployed to Preview – media-chrome April 17, 2023 23:02 View deployment

vercel bot deployed to Preview – media-chrome-docs April 17, 2023 23:03 View deployment

remove hardcoded declaration file (with react types) (at least for now).

f832f82

vercel bot had a problem deploying to Preview – media-chrome April 17, 2023 23:04 Failure

vercel bot had a problem deploying to Preview – media-chrome-docs April 17, 2023 23:04 Failure

chore: minimal changes to media-playback-rate-button rates for TS fixes.

39bace6

vercel bot had a problem deploying to Preview – media-chrome-docs April 17, 2023 23:17 Failure

vercel bot had a problem deploying to Preview – media-chrome April 17, 2023 23:17 Failure

fix: remove script to copy (no longer present) type declaration file …

2ade40d

…on build.

cjpillsbury commented Apr 18, 2023

View reviewed changes

vercel bot deployed to Preview – media-chrome April 18, 2023 14:15 View deployment

vercel bot deployed to Preview – media-chrome-docs April 18, 2023 14:15 View deployment

cjpillsbury marked this pull request as ready for review April 18, 2023 14:24

cjpillsbury requested review from a team and heff as code owners April 18, 2023 14:24

luwes approved these changes Apr 18, 2023

View reviewed changes

gkatsev reviewed Apr 18, 2023

View reviewed changes

remove props from media-time-display and clean up media-container css…

c0ffeb6

… formatting.

vercel bot deployed to Preview – media-chrome April 18, 2023 19:51 View deployment

vercel bot deployed to Preview – media-chrome-docs April 18, 2023 19:51 View deployment

cjpillsbury merged commit fe9eadc into muxinc:main Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Migrate all attributes to lowercase ('smushedcase'). #537

feat: Migrate all attributes to lowercase ('smushedcase'). #537

cjpillsbury commented Apr 17, 2023 •

edited

Loading

vercel bot commented Apr 17, 2023 •

edited

Loading

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

luwes Apr 18, 2023

cjpillsbury Apr 18, 2023

gkatsev Apr 18, 2023

cjpillsbury Apr 18, 2023

cjpillsbury Apr 17, 2023

luwes Apr 18, 2023

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

cjpillsbury Apr 17, 2023

luwes Apr 17, 2023

cjpillsbury Apr 17, 2023

luwes Apr 17, 2023

cjpillsbury Apr 18, 2023

gkatsev Apr 17, 2023

cjpillsbury Apr 17, 2023

codecov bot commented Apr 17, 2023 •

edited

Loading

cjpillsbury Apr 18, 2023

luwes left a comment

gkatsev Apr 18, 2023

gkatsev Apr 18, 2023

cjpillsbury Apr 18, 2023

gkatsev Apr 18, 2023

		@@ -0,0 +1,200 @@
		export * as constants from "./constants.js";

feat: Migrate all attributes to lowercase ('smushedcase'). #537

feat: Migrate all attributes to lowercase ('smushedcase'). #537

Conversation

cjpillsbury commented Apr 17, 2023 • edited Loading

vercel bot commented Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

luwes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpillsbury commented Apr 17, 2023 •

edited

Loading

vercel bot commented Apr 17, 2023 •

edited

Loading

codecov bot commented Apr 17, 2023 •

edited

Loading