Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📦 Bundle Size Report🎨 @videojs/html — no changesPresets (7)
Media (8)
Players (3)
Skins (17)
UI Components (25)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (7)
Skins (14)
UI Components (20)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (9)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (3)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
cjpillsbury
left a comment
cjpillsbury
left a comment
There was a problem hiding this comment.
Although I threw out a few "blocking" comments, I trust the rest of the team to address/discuss sufficiently and I will be out for a long weekend and don't want to slow forward progress, so setting this review as "Comment" only.
cjpillsbury
left a comment
cjpillsbury
left a comment
There was a problem hiding this comment.
Only one documentation-centric callout. Otherwise I feel like this addresses all of my major concerns. Looking good.
There was a problem hiding this comment.
Awesome 😎 I've left a bunch of comments but overall looking really good. In large I was just pressing on where this should live and stronger types.
I might've missed it but I didn't catch how this will all tie into updating primitive component language (say label on a play button). I can imagine few obvious ways off the top of my head so not concerned.
|
I've created tickets based on the current state of the document but I can incorporate any further feedback as required. |
| import es from '@videojs/html/i18n/locales/es'; | ||
|
|
||
| // React | ||
| import { registerI18n } from '@videojs/react'; |
There was a problem hiding this comment.
nitpick: import path inconsistencies between react vs. html (assuming a typo)
| import es from '@videojs/react/i18n/locales/es'; | ||
|
|
||
| <Provider> | ||
| <I18nProvider locale="es" translations={es}> |
There was a problem hiding this comment.
question(non-blocking): Is there a reason for the lang vs. locale discrepancy?
There was a problem hiding this comment.
As per our discussions, the lang attribute makes more sense in HTML but in the React world, locale seems to be more used. I'm happy to make it lang to be consistent though if you'd prefer? I don't feel super strongly, Claude just suggested it was more idiomatic for React devs.
|
|
||
| ```ts | ||
| // @videojs/core/i18n/registry.ts | ||
| const registry = new Map<string, Partial<Translations>>(); |
There was a problem hiding this comment.
suggestion(non-blocking): If we're going to have stateful/mutative modules (roughly equivalent to a "capital S Singleton"), let's make sure minimally we can easily remove registered languages for testing?
| if (!locale) { setBuiltIn({}); return; } | ||
| const tags = [...new Set([locale, locale.split('-')[0]])]; | ||
| const load = (i = 0): Promise<void> => | ||
| import(`@videojs/core/i18n/locales/${tags[i]}`) |
There was a problem hiding this comment.
thought(non-blocking): We may need to do some validation of this strategy in real world app build setups, but I don't see any problem starting with this hope/dream. Let's just make sure we do enough smoke testing of this that we don't think it will break folks doing normal crud with normal frameworks/build tools in the wild.
| /** Resolve effective locale: explicit lang attribute → nearest ancestor[lang] → documentElement.lang → undefined. */ | ||
| get #effectiveLocale(): Locale | undefined { | ||
| if (this.lang) return this.lang; | ||
| const ancestor = this.parentElement?.closest('[lang]'); |
There was a problem hiding this comment.
thought(non-blocking): Can't remember if we need to account for shadow dom boundaries with this, but we have prior art for that kinda stuff, so we can always update accordingly if/when needed.
cjpillsbury
left a comment
cjpillsbury
left a comment
There was a problem hiding this comment.
This all looks great. Some nb callouts, but I'm comfortable with moving forward here. One high level callout, re: implementation - we may be able to pull out some of this functionality on the react and html side into shareable code (e.g. the MutationObserver stuff). I don't think that needs to be updated in the RFC, but food for thought on maintainability and/or bundle size considerations (though, for the later, it's unlikely there will be a mix and match of html + react usages in real world examples in the wild).
|
I left this issue of elements that might be related to this discussion. |
7 participants
Ref: #222
Summary
Add a design document for the i18n (internationalization) system, capturing the architecture decisions and rationale for how Video.js 10 will handle localization across its monorepo packages.
Closes #1381
Changes
Testing
Documentation only — no code changes.
Note
Low Risk
Low risk documentation-only addition plus a minor
.gitignoreupdate; no runtime behavior or APIs are changed in this PR.Overview
Adds a comprehensive draft design document (
internal/design/i18n.md) specifying the proposed Video.js 10 internationalization architecture and APIs (registry-based translation packs, HTML/React provider patterns, locale fallback rules, and CDN-friendly locale modules).Updates
.gitignoreto also ignore pnpm’s local store directory (.pnpm-store/).Reviewed by Cursor Bugbot for commit fc2ca7e. Bugbot is set up for automated code reviews on this repo. Configure here.