fix(lit): accept partial hinted text styles#832
fix(lit): accept partial hinted text styles#832stablegenius49 wants to merge 1 commit intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the lit text renderer required all hinted text style keys to be present, and now correctly accepts partial hinted text styles. The logic is updated to check for at least one valid key, and the related types and functions have been refactored into a new file. A regression test has also been added, which is great. The changes are solid, but I've left one comment regarding a mismatch in a type definition that could be improved for better type safety.
| export interface HintedStyles { | ||
| h1: Record<string, string>; | ||
| h2: Record<string, string>; | ||
| h3: Record<string, string>; | ||
| h4: Record<string, string>; | ||
| h5: Record<string, string>; | ||
| body: Record<string, string>; | ||
| caption: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
The isHintedStyles type guard correctly validates partially defined hinted styles, but the HintedStyles interface requires all properties to be present. This creates a mismatch where the type system will incorrectly assume all properties exist after the type guard passes. To make the type definition accurate and prevent potential runtime errors, all properties on HintedStyles should be optional.
| export interface HintedStyles { | |
| h1: Record<string, string>; | |
| h2: Record<string, string>; | |
| h3: Record<string, string>; | |
| h4: Record<string, string>; | |
| h5: Record<string, string>; | |
| body: Record<string, string>; | |
| caption: Record<string, string>; | |
| } | |
| export interface HintedStyles { | |
| h1?: Record<string, string>; | |
| h2?: Record<string, string>; | |
| h3?: Record<string, string>; | |
| h4?: Record<string, string>; | |
| h5?: Record<string, string>; | |
| body?: Record<string, string>; | |
| caption?: Record<string, string>; | |
| } |
Summary
additionalStyles.Textas hinted stylesh6expectation and treat partial hinted style maps as hinted when they contain at least one recognized hinted keyTesting
cd renderers/web_core && npm run buildcd renderers/lit && npm testFixes #602