feat: add support for media queries #261
Conversation
📝 WalkthroughWalkthroughThis change implements support for CSS variables within media queries in Uniwind. The system tracks variable declarations tied to media query conditions (minWidth, maxWidth, orientation, colorScheme), stores them during CSS compilation, and resolves the appropriate variable value at runtime based on current device state (screen dimensions, orientation, theme). Changes
Sequence Diagram(s)sequenceDiagram
participant CSS as CSS Input
participant Proc as Processor
participant MQ as MediaQueryResolver
participant Compile as compileVirtual
participant Store as UniwindStore
participant Runtime as Runtime Env
rect rgb(200, 220, 255)
note over CSS,Compile: Compilation Phase
CSS->>Proc: CSS with variables in `@media`
Proc->>MQ: processMediaQueries()
MQ-->>Proc: MediaQueryResolver (conditions)
Proc->>Proc: storeVarWithMediaQuery(varName, value, mq)
Proc->>Proc: Accumulate in varsWithMediaQueries
Proc->>Compile: Emit varsWithMediaQueries
Compile->>Compile: serialize variants
Compile-->>Runtime: Include varsWithMediaQueries in payload
end
rect rgb(200, 255, 200)
note over Store,Runtime: Runtime Resolution Phase
Runtime->>Store: reinit(config with varsWithMediaQueries)
Store->>Store: Store varsWithMediaQueries per variable
Runtime->>Runtime: Device state changes (width, orientation, colorScheme)
Runtime->>Store: resolveStyles(classNames)
Store->>Store: resolveMediaQueryVars()
Store->>Store: Match variants (max minWidth fitting conditions)
Store-->>Store: Compute overridden variable values
Store->>Store: Apply vars to class resolution
Store-->>Runtime: Resolved styles
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/uniwind/src/metro/processor/processor.ts (2)
108-171: Extract duplicatedhasMediaQuerycondition.The same condition is repeated three times (lines 113, 136, 158). This violates DRY and increases maintenance burden.
🔎 Suggested refactor
Calculate
hasMediaQueryonce near the top ofaddDeclaration, aftermqis computed:private addDeclaration(declaration: Declaration, important = false) { const isVar = this.declarationConfig.root || this.declarationConfig.className === null const mq = this.MQ.processMediaQueries(this.declarationConfig.mediaQueries) + const hasMediaQuery = isVar && ( + mq.minWidth !== 0 || + mq.maxWidth !== Number.MAX_VALUE || + mq.orientation !== null || + mq.colorScheme !== null + ) const style = (() => {Then use it directly in each branch:
if (isVar) { - const hasMediaQuery = mq.minWidth !== 0 || mq.maxWidth !== Number.MAX_VALUE || mq.orientation !== null || mq.colorScheme !== null - if (hasMediaQuery) { this.storeVarWithMediaQuery(varName, processedValue, mq) } else {
274-289: Remove the dead code splice or reposition the config reset.The
declarationConfigreset on line 280 creates a fresh object with an emptymediaQueriesarray. The subsequent splice (lines 283-286) operates on this empty array, making it a no-op. Since declarations are processed before the reset, the splice provides no value and should be removed. Alternatively, move the reset outside and after the forEach loop if cleanup is intended.
♻️ Duplicate comments (3)
packages/uniwind/tests/media-queries/max-width.test.tsx (1)
26-30: Sameeval()usage as other test files.Apply the same refactoring pattern suggested for
combined.test.tsxto use a safer initialization approach.packages/uniwind/tests/media-queries/min-width.test.tsx (1)
26-30: Sameeval()usage as other test files.Apply the same refactoring pattern suggested for
combined.test.tsx.packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx (1)
26-30: Sameeval()usage as other test files.Apply the same refactoring pattern suggested for
combined.test.tsx.
🧹 Nitpick comments (7)
packages/uniwind/tests/media-queries/min-width.test.css (1)
14-33: Review the media query ordering for clarity.The media queries are defined in non-ascending order: 640px (line 14), 1024px (line 21), then 320px (line 29). Due to CSS cascade rules, when multiple media queries match simultaneously, the last one in source order takes precedence.
For example, at width 700px, both the 640px and 320px breakpoints match. Since 320px appears later in the source, its
--text-base: 1.5remwill override the 640px rule's--text-base: 2rem, which is likely unintended.If this ordering is intentional for testing edge cases, consider adding a comment to clarify. Otherwise, reorder them in ascending sequence.
🔎 Suggested reordering
+@media (min-width: 320px) { + :root { + --text-base: 1.5rem; + } +} + @media (min-width: 640px) { :root { --text-base: 2rem; --text-lg: 3rem; } } @media (min-width: 1024px) { :root { --text-base: 3rem; --text-lg: 4rem; --text-xl: 5rem; } } - -@media (min-width: 320px) { - :root { - --text-base: 1.5rem; - } -}packages/uniwind/tests/media-queries/color-scheme.test.tsx (1)
26-30: Consider alternatives to eval() for test setup.While using
eval()in tests is less critical than in production code, it still poses security and maintainability concerns. The static analyzer correctly flagged this pattern.If the test framework supports it, consider using dynamic
import()or extracting the compiled code to a temporary module file that can be required normally.packages/uniwind/tests/media-queries/orientation.test.tsx (1)
26-30: Consider alternatives to eval() for test initialization.Similar to the color-scheme test file, the
eval()usage here is flagged by static analysis. While acceptable in test contexts, exploring safer alternatives like dynamic imports or temporary module files would improve maintainability.packages/uniwind/src/metro/compileVirtual.ts (1)
62-64: Consider handling emptyvarsWithMediaQueriesgracefully.When
Processor.varsWithMediaQueriesis empty, this produces an empty string forvarsWithMediaQueries, resulting invarsWithMediaQueries: ({ }),in the output. While this works, it's slightly inconsistent with how other empty cases might be handled.🔎 Optional: Add explicit empty object fallback
const varsWithMediaQueries = Object.entries(Processor.varsWithMediaQueries) .map(([key, value]) => `"${key}": ${serialize(value)}`) - .join(',') + .join(',') || ''packages/uniwind/src/core/native/store.ts (1)
117-126: Potential stale closure capture ofbestMatch.The getter at line 124 captures
bestMatchby reference. SincebestMatchis reassigned in the loop, all getters will return the value from the lastbestMatchassignment for that variable, which is correct. However, if JavaScript engines optimize this differently, consider capturing the value explicitly.🔎 Explicit value capture for clarity
if (bestMatch !== null) { + const resolvedValue = bestMatch.value Object.defineProperty(vars, varName, { configurable: true, enumerable: true, - get: () => bestMatch.value, + get: () => resolvedValue, }) }packages/uniwind/tests/media-queries/combined.test.tsx (1)
25-29: Consider replacingeval()with a safer alternative for test initialization.While
eval()is acceptable in test code with controlled inputs, the static analysis tool flags this as a security concern. Consider creating a shared test helper that uses theFunctionconstructor or a dedicated module loader pattern to avoid the lint warning and improve maintainability across all media query test files.🔎 Safer alternative using Function constructor
- eval( - `const { Uniwind } = require('../../src/core/config/config.native'); - Uniwind.__reinit(rt => ${virtualCode}, ['light', 'dark']); - `, - ) + const { Uniwind } = require('../../src/core/config/config.native') + const initFn = new Function('rt', `return ${virtualCode}`) + Uniwind.__reinit(initFn, ['light', 'dark'])Or extract to a shared helper in
tests/utils.ts:export const initUniwindWithVirtualCode = (virtualCode: string, themes: string[]) => { const { Uniwind } = require('../../src/core/config/config.native') const initFn = new Function('rt', `return ${virtualCode}`) Uniwind.__reinit(initFn, themes) }packages/uniwind/src/metro/processor/processor.ts (1)
15-15: Consider defining a specific type for media query variable entries.The
anytype loses type safety for the array elements. Since each entry has a known shape (value,minWidth,maxWidth,orientation,colorScheme), consider defining an interface.🔎 Suggested type definition
// In types.ts or locally interface MediaQueryVarEntry { value: any minWidth: number | null maxWidth: number | null orientation: string | null colorScheme: string | null }Then use:
- varsWithMediaQueries = {} as Record<string, Array<any>> + varsWithMediaQueries = {} as Record<string, Array<MediaQueryVarEntry>>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/uniwind/src/core/native/store.tspackages/uniwind/src/core/types.tspackages/uniwind/src/metro/compileVirtual.tspackages/uniwind/src/metro/processor/mq.tspackages/uniwind/src/metro/processor/processor.tspackages/uniwind/src/metro/types.tspackages/uniwind/tests/media-queries/color-scheme.test.csspackages/uniwind/tests/media-queries/color-scheme.test.tsxpackages/uniwind/tests/media-queries/combined.test.csspackages/uniwind/tests/media-queries/combined.test.tsxpackages/uniwind/tests/media-queries/css-variables-media-queries.test.csspackages/uniwind/tests/media-queries/css-variables-media-queries.test.tsxpackages/uniwind/tests/media-queries/max-width.test.csspackages/uniwind/tests/media-queries/max-width.test.tsxpackages/uniwind/tests/media-queries/min-width.test.csspackages/uniwind/tests/media-queries/min-width.test.tsxpackages/uniwind/tests/media-queries/orientation.test.csspackages/uniwind/tests/media-queries/orientation.test.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
packages/uniwind/src/metro/processor/processor.ts (2)
packages/uniwind/src/metro/types.ts (1)
MediaQueryResolver(29-42)packages/uniwind/src/metro/processor/var.ts (1)
Var(4-18)
packages/uniwind/tests/media-queries/min-width.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-78)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(272-272)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
packages/uniwind/tests/media-queries/orientation.test.css (2)
packages/uniwind/src/core/config/config.native.ts (1)
updateCSSVariables(25-68)packages/uniwind/src/core/config/config.ts (1)
updateCSSVariables(32-50)
packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-78)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(272-272)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)
packages/uniwind/tests/media-queries/max-width.test.tsx (5)
packages/uniwind/src/metro/compileVirtual.ts (1)
compileVirtual(21-78)packages/uniwind/src/components/index.ts (1)
Platform(193-195)packages/uniwind/src/core/native/store.ts (1)
UniwindStore(272-272)packages/uniwind/src/core/listener.ts (1)
UniwindListener(59-59)packages/uniwind/tests/utils.ts (1)
renderUniwind(6-18)
packages/uniwind/src/metro/processor/mq.ts (1)
packages/uniwind/src/metro/types.ts (1)
MediaQueryResolver(29-42)
packages/uniwind/src/metro/compileVirtual.ts (1)
packages/uniwind/src/metro/utils/serialize.ts (1)
serialize(48-91)
🪛 Biome (2.1.2)
packages/uniwind/tests/media-queries/orientation.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/color-scheme.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/min-width.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/combined.test.tsx
[error] 25-25: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/uniwind/tests/media-queries/max-width.test.tsx
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (20)
packages/uniwind/tests/media-queries/css-variables-media-queries.test.css (1)
1-16: Verify that companion Jest tests validate variable resolution under media query conditions.This CSS fixture is well-structured and correctly tests the scenario from issue #242 (CSS variables redefined in media queries). However, this file alone is a CSS input fixture and lacks test assertions.
Key verification needs:
Companion test code: Please confirm that a corresponding Jest test file (e.g.,
css-variables-media-queries.test.tsorcss-variables-media-queries.test.js) exists and:
- Loads this CSS fixture
- Parses the media query declarations
- Validates that variables resolve correctly based on media conditions
- Asserts that
--text-baseand--text-lgresolve to the overridden values (2rem, 3rem) when the min-width condition is metCoverage of all media query types: The AI summary mentions support for
minWidth,maxWidth,orientation, andcolorScheme. This test file only coversmin-width: 640px. Are additional test fixtures and tests covering the other media query types?Runtime resolution validation: Ensure tests verify that the runtime variable resolution respects the current device state (screen width, orientation, theme), not just parse the CSS structure.
Please share the companion Jest test file(s) or provide a verification script showing how this CSS fixture is tested.
packages/uniwind/tests/media-queries/orientation.test.css (1)
1-25: LGTM! Well-structured test fixture for orientation-based variable overrides.The CSS module correctly defines baseline theme variables and overrides them within a landscape media query, providing a clean test case for validating orientation-aware variable resolution.
packages/uniwind/tests/media-queries/max-width.test.css (1)
1-30: LGTM! Clean test fixture for max-width responsive variables.The CSS correctly implements smaller spacing and text values at the 480px breakpoint, following mobile-first best practices for responsive design testing.
packages/uniwind/tests/media-queries/color-scheme.test.css (1)
1-19: LGTM! Focused test case for color scheme variable handling.The CSS provides a minimal, clear test scenario with contrasting colors for light and dark modes.
packages/uniwind/tests/media-queries/min-width.test.css (1)
35-53: LGTM! Utility classes correctly reference theme variables.The utility classes are well-structured and properly consume the CSS variables defined in the theme layer.
packages/uniwind/src/metro/types.ts (1)
29-42: Excellent type safety improvement!Refining
maxWidthandminWidthfromanytonumber | nulladds proper type constraints and makes the API contract clearer. Thenulloption appropriately handles cases where no width constraint is specified.packages/uniwind/tests/media-queries/combined.test.css (1)
1-30: LGTM! Comprehensive test for combined media query conditions.The CSS correctly exercises compound media queries with multiple conditions (
andoperator), validating that the framework can handle complex responsive scenarios where width, orientation, and color-scheme interact.packages/uniwind/tests/media-queries/color-scheme.test.tsx (1)
41-83: LGTM! Comprehensive test coverage for color scheme media queries.The test suite thoroughly validates:
- Default light mode behavior
- Dark mode variable resolution
- Dynamic color scheme updates with proper re-rendering
The assertions correctly verify color values and the test structure properly isolates state with
beforeEach.packages/uniwind/tests/media-queries/orientation.test.tsx (2)
103-113: Excellent edge case coverage for media query precedence.This test validates a critical behavior: when both orientation and min-width conditions could apply separately, orientation-based variable overrides take precedence. This helps ensure consistent and predictable resolution logic.
39-101: LGTM! Thorough test coverage for orientation media queries.The test suite comprehensively validates:
- Portrait default values
- Landscape overrides
- Dynamic orientation changes with proper reactivity
- Multiple variable handling within the same orientation context
All assertions are meaningful and test setup properly isolates state.
packages/uniwind/src/core/types.ts (1)
28-37: LGTM!The
varsWithMediaQueriestype definition is well-structured and consistent with theMediaQueryResolvertype inmetro/types.ts. Making it optional ensures backward compatibility with existing code that doesn't use media-query-scoped variables.packages/uniwind/src/metro/compileVirtual.ts (1)
70-77: LGTM!The integration of
varsWithMediaQueriesinto the virtual module output is clean and follows the established pattern for other serialized properties.packages/uniwind/src/core/native/store.ts (2)
108-115: Verify the comparison ofcolorSchemewithcurrentThemeName.Line 112 compares
variant.colorSchemeagainstthis.runtime.currentThemeName. This works if theme names match color scheme values ('light'/'dark'), but may not work for custom theme names that don't correspond directly to color schemes.Is this intentional, or should it compare against
this.runtime.colorSchemeinstead?
93-130: Consider extracting media query matching logic.The
resolveMediaQueryVarsmethod handles both dependency tracking and value resolution. For maintainability, consider extracting the matching logic into a separate helper, especially as media query complexity grows.Overall, the implementation correctly:
- Tracks dependencies for reactive updates
- Handles multiple media query conditions
- Selects the best match using highest
minWidthprioritypackages/uniwind/tests/media-queries/combined.test.tsx (1)
40-120: Good test coverage for combined media query scenarios.The test cases comprehensively cover:
- Combined min-width + orientation conditions
- Combined min-width + color-scheme conditions
- Fallback behavior when partial conditions match
- Complex multi-condition scenarios
packages/uniwind/tests/media-queries/max-width.test.tsx (1)
87-125: Well-structured dynamic update tests.The tests properly verify reactivity when screen dimensions change across the max-width threshold in both directions, using
act()appropriately for state updates.packages/uniwind/tests/media-queries/min-width.test.tsx (1)
39-187: Excellent test coverage for min-width media queries.The test suite thoroughly covers:
- Default values below thresholds
- Values at exact breakpoint boundaries
- Highest matching breakpoint selection
- Multiple variables with same/different breakpoints
- Dynamic updates across multiple breakpoints
- Edge cases (very small/large widths)
packages/uniwind/src/metro/processor/mq.ts (1)
9-25: Good refactor to return the resolver.Returning
mqfromprocessMediaQueriesenables callers to access the resolved media query constraints, which is essential for the newvarsWithMediaQueriesfeature.packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx (1)
86-142: Good coverage of dynamic screen width changes and fallback behavior.The dynamic update tests properly use
act()and include the notify calls. The fallback test correctly verifies default values when no media query matches.packages/uniwind/src/metro/processor/processor.ts (1)
58-70: LGTM!The helper method correctly initializes the array lazily and stores the media query conditions with appropriate string quoting for runtime evaluation.
| private processCondition(condition: MediaCondition, mq: MediaQueryResolver) { | ||
| if (condition.type === 'operation') { | ||
| condition.conditions.forEach(nestedCondition => { | ||
| this.processCondition(nestedCondition, mq) | ||
| }) | ||
| } else if (condition.type === 'feature') { | ||
| if (condition.value.type === 'range') { | ||
| this.processWidthMediaQuery(condition.value, mq) | ||
| } | ||
|
|
||
| if (condition.value.type === 'plain') { | ||
| this.processPlainMediaQuery(condition.value, mq) | ||
| } | ||
| }) | ||
|
|
||
| return mq | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MediaCondition import and type definition
rg -n "MediaCondition" packages/uniwind/src/metro/processor/mq.ts -C 2Repository: uni-stack/uniwind
Length of output: 482
🏁 Script executed:
#!/bin/bash
# Search for Lightning CSS imports and any type definitions in the project
rg -n "from.*lightningcss" packages/uniwind/ --type tsRepository: uni-stack/uniwind
Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Check if there are any type definition files or interfaces for MediaCondition
fd -e d.ts -e ts | head -20 | xargs grep -l "MediaCondition" 2>/dev/null || echo "No type definition files found"Repository: uni-stack/uniwind
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about condition types
rg -n "condition\.type" packages/uniwind/ --type ts -C 1Repository: uni-stack/uniwind
Length of output: 647
🌐 Web query:
Lightning CSS MediaCondition types operation feature not
💡 Result:
Short answer: Lightning CSS represents parsed media queries with a MediaCondition union whose variants are objects with a type field such as "type", "feature", "operation", and "not" (plus a few others like "style"/container-related variants). Each variant means:
- type — a media type node (e.g., "screen", "print"). [1]
- feature — a media feature or container feature (e.g., (min-width: 600px)); the node contains the feature name and value. [2]
- operation — a logical grouping with an operator (and/or) and an array of sub-conditions (used for expressions like (A and B)). [2]
- not — a negation node wrapping a single condition (e.g., not screen). [2]
See Lightning CSS docs and the TypeScript AST typings (lightningcss-wasm) for the exact shapes of the nodes. [1][2]
Sources:
- Lightning CSS docs / crate (media-query and AST modules). [1]
- lightningcss-wasm TypeScript AST diff / d.ts showing MediaCondition variants ("type", "feature", "operation", "not", etc.). [2]
🌐 Web query:
lightningcss MediaCondition type definition "operation" "feature" "not"
💡 Result:
In Lightning CSS the MediaCondition AST (media_query::MediaCondition) has three node kinds you asked about:
-
type: "feature" — a single media/container feature (e.g. (width >= 600px) or (prefers-reduced-motion: reduce)). Represents a QueryFeature node containing the feature name and value. [1]
-
type: "operation" — a boolean operation combining conditions using "and" or "or" (e.g. (min-width: 600px) and (orientation: landscape)). Holds the operator and left/right child MediaCondition nodes. [1]
-
type: "not" — a negation of a condition (e.g. not (hover: hover)). Contains the single child MediaCondition being negated. [1]
Sources: docs for lightningcss media_query (MediaCondition / QueryFeature). [1][2]
References
[1] lightningcss media_query (MediaCondition / QueryFeature) — docs.rs. (docs.rs)
[2] lightningcss project README (features list: media queries support) — GitHub. (github.com)
Citations:
- 1: https://docs.rs/lightningcss/latest/lightningcss/all.html?utm_source=openai
- 2: https://github.com/parcel-bundler/lightningcss?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or other usages of processCondition to understand expected behavior
rg -n "processCondition\|MediaCondition" packages/uniwind/ --type ts -B 2 -A 5 | head -60Repository: uni-stack/uniwind
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for any handling of 'not' or 'type' condition types in the entire codebase
rg -n "condition\.type.*=.*['\"]not['\"]|condition\.type.*=.*['\"]type['\"]" packages/uniwind/ --type tsRepository: uni-stack/uniwind
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the full context of the processCondition method and surrounding code
cat -n packages/uniwind/src/metro/processor/mq.ts | head -50Repository: uni-stack/uniwind
Length of output: 2202
Handle missing MediaCondition types: 'not' and 'type'.
The current implementation only processes 'operation' and 'feature' condition types, but Lightning CSS MediaCondition is a union that also includes 'type' (media type constraints like screen, print) and 'not' (negations like not (hover)). These types are silently ignored, which means media queries using negations or type constraints won't be processed correctly.
Add handling for both:
'not'type: recursively process the wrapped condition'type'type: decide whether media type constraints need processing or can be skipped with a comment explaining why
| test('uses default value when screen width is below media query threshold', () => { | ||
| UniwindStore.runtime.screen = { width: 400, height: 667 } | ||
|
|
||
| const { getStylesFromId } = renderUniwind( | ||
| <View className="text-base" testID="text-base" />, | ||
| ) | ||
|
|
||
| expect(getStylesFromId('text-base').fontSize).toBe(16) | ||
| }) |
There was a problem hiding this comment.
Missing UniwindListener.notify() after screen dimension change.
Unlike other tests in this file (lines 51-52, 88-89) and in other test files, this test changes runtime.screen but doesn't call UniwindListener.notify([StyleDependency.Dimensions]). This inconsistency could lead to unreliable test behavior if the cache isn't invalidated.
🔎 Proposed fix
test('uses default value when screen width is below media query threshold', () => {
UniwindStore.runtime.screen = { width: 400, height: 667 }
+ UniwindListener.notify([StyleDependency.Dimensions])
const { getStylesFromId } = renderUniwind(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('uses default value when screen width is below media query threshold', () => { | |
| UniwindStore.runtime.screen = { width: 400, height: 667 } | |
| const { getStylesFromId } = renderUniwind( | |
| <View className="text-base" testID="text-base" />, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(16) | |
| }) | |
| test('uses default value when screen width is below media query threshold', () => { | |
| UniwindStore.runtime.screen = { width: 400, height: 667 } | |
| UniwindListener.notify([StyleDependency.Dimensions]) | |
| const { getStylesFromId } = renderUniwind( | |
| <View className="text-base" testID="text-base" />, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(16) | |
| }) |
🤖 Prompt for AI Agents
In packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx
around lines 40 to 48, the test updates UniwindStore.runtime.screen but does not
call UniwindListener.notify([StyleDependency.Dimensions]) to invalidate caches;
add a call to UniwindListener.notify([StyleDependency.Dimensions]) immediately
after setting the screen dimensions so the style cache is refreshed before
rendering and assertions.
| test('uses media query value when screen width equals threshold', () => { | ||
| UniwindStore.runtime.screen = { width: 640, height: 667 } | ||
|
|
||
| const { getStylesFromId } = renderUniwind( | ||
| <View className="text-base" testID="text-base" />, | ||
| ) | ||
|
|
||
| expect(getStylesFromId('text-base').fontSize).toBe(32) | ||
| }) |
There was a problem hiding this comment.
Missing UniwindListener.notify() after screen dimension change.
Same issue as line 41 - the screen dimensions are changed but the listener isn't notified.
🔎 Proposed fix
test('uses media query value when screen width equals threshold', () => {
UniwindStore.runtime.screen = { width: 640, height: 667 }
+ UniwindListener.notify([StyleDependency.Dimensions])
const { getStylesFromId } = renderUniwind(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('uses media query value when screen width equals threshold', () => { | |
| UniwindStore.runtime.screen = { width: 640, height: 667 } | |
| const { getStylesFromId } = renderUniwind( | |
| <View className="text-base" testID="text-base" />, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(32) | |
| }) | |
| test('uses media query value when screen width equals threshold', () => { | |
| UniwindStore.runtime.screen = { width: 640, height: 667 } | |
| UniwindListener.notify([StyleDependency.Dimensions]) | |
| const { getStylesFromId } = renderUniwind( | |
| <View className="text-base" testID="text-base" />, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(32) | |
| }) |
🤖 Prompt for AI Agents
In packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx
around lines 61 to 69, the test mutates UniwindStore.runtime.screen but doesn't
notify listeners, so runtime changes won't propagate; after setting
UniwindStore.runtime.screen = { width: 640, height: 667 } call
UniwindListener.notify() (or the appropriate notify method used elsewhere in
tests) to trigger updates before rendering and asserting.
| test('handles multiple variables with media queries', () => { | ||
| UniwindStore.runtime.screen = { width: 800, height: 667 } | ||
|
|
||
| const { getStylesFromId } = renderUniwind( | ||
| <React.Fragment> | ||
| <View className="text-base" testID="text-base" /> | ||
| <View className="text-lg" testID="text-lg" /> | ||
| </React.Fragment>, | ||
| ) | ||
|
|
||
| expect(getStylesFromId('text-base').fontSize).toBe(32) | ||
| expect(getStylesFromId('text-lg').fontSize).toBe(48) | ||
| }) |
There was a problem hiding this comment.
Missing UniwindListener.notify() after screen dimension change.
Same issue - add the notify call for consistency.
🔎 Proposed fix
test('handles multiple variables with media queries', () => {
UniwindStore.runtime.screen = { width: 800, height: 667 }
+ UniwindListener.notify([StyleDependency.Dimensions])
const { getStylesFromId } = renderUniwind(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('handles multiple variables with media queries', () => { | |
| UniwindStore.runtime.screen = { width: 800, height: 667 } | |
| const { getStylesFromId } = renderUniwind( | |
| <React.Fragment> | |
| <View className="text-base" testID="text-base" /> | |
| <View className="text-lg" testID="text-lg" /> | |
| </React.Fragment>, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(32) | |
| expect(getStylesFromId('text-lg').fontSize).toBe(48) | |
| }) | |
| test('handles multiple variables with media queries', () => { | |
| UniwindStore.runtime.screen = { width: 800, height: 667 } | |
| UniwindListener.notify([StyleDependency.Dimensions]) | |
| const { getStylesFromId } = renderUniwind( | |
| <React.Fragment> | |
| <View className="text-base" testID="text-base" /> | |
| <View className="text-lg" testID="text-lg" /> | |
| </React.Fragment>, | |
| ) | |
| expect(getStylesFromId('text-base').fontSize).toBe(32) | |
| expect(getStylesFromId('text-lg').fontSize).toBe(48) | |
| }) |
🤖 Prompt for AI Agents
In packages/uniwind/tests/media-queries/css-variables-media-queries.test.tsx
around lines 71 to 83, the test updates UniwindStore.runtime.screen but does not
call UniwindListener.notify() afterward, so the runtime change isn't propagated;
after setting UniwindStore.runtime.screen = { width: 800, height: 667 } add a
call to UniwindListener.notify() (or the appropriate notify method used in the
test suite) before rendering to ensure listeners pick up the new screen
dimensions.
| } | ||
| } | ||
|
|
||
| private resolveMediaQueryVars(dependencies: Set<StyleDependency>) { |
There was a problem hiding this comment.
@Brentlok Kindly review this:
The coderabbitai has reviewed most part of the code, but I'll need an in-dept review of this method resolving media queries at runtime
|
Hi there, thanks for the submission! I genuinely appreciate the time and heavy lifting you put into this. It’s clearly a massive undertaking. Unfortunately, I have to close this PR. As I mentioned previously (and as outlined in our README), we require a discussion and approval process for large architectural changes before the code is written. This rule exists primarily to protect your time - we never want you to spend days working on something that we can't merge. Currently, this PR introduces complexity and performance costs that we aren't ready to take on. Since this feature wasn't on our roadmap, and the implementation differs significantly from our coding standards, we won't be able to review it. For next time, please ping us in an issue first. We’d love to brainstorm ideas with you there to ensure your next contribution gets merged smoothly! |
2 participants
fixes #242
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.