fix(fluent-editor): [fluent-editor] change theme #3051
Conversation
WalkthroughThis pull request updates the styling for the Fluent Editor component. It introduces a new LESS file that defines CSS custom properties scoped to the component and updates the main styling file to import these variables and apply them via a mixin. The modifications adjust the toolbar, picker, and icon styles by replacing hardcoded values with variable-driven definitions, enabling more flexible theming. Additionally, several new test cases are added to verify the functionality and behavior of the Fluent Editor in various scenarios. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
examples/sites/demos/pc/app/fluent-editor/data-switch.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThis PR introduces a theme change for the fluent-editor in the TinyVue project. It primarily involves importing a new variables file and updating the styling of the editor components to use CSS variables for colors, borders, and other stylistic elements, enhancing the theme's flexibility and maintainability. Changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/theme/src/fluent-editor/index.less (1)
1-321: Reminder: Add Visual Regression Tests and Documentation Updates:
As noted in the PR objectives, tests and documentation updates have not been provided with these theme changes. It is advisable to include visual regression tests or snapshots to capture the new styling and update the component’s style guide documentation to reflect the usage of new CSS custom properties.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/theme/src/fluent-editor/index.less(3 hunks)packages/theme/src/fluent-editor/vars.less(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/fluent-editor/vars.less
🔇 Additional comments (8)
packages/theme/src/fluent-editor/index.less (8)
3-3: New Import for CSS Variables:
The addition of@import './vars.less';imports essential CSS custom properties for the Fluent Editor theme. Ensure thatvars.lessdefines all the required variables (colors, border radius, shadows, etc.) to maintain consistency across the component.
5-6: Prefix Definitions with Interpolation:
The definitions for@editor-wrapper-prefix-clsand@editor-prefix-clsusing tilde string interpolation ensure dynamic class naming based on the global@{css-prefix}variable. Double-check that@{css-prefix}is defined in the global scope so that the generated class names are correct.
8-10: Mixin Invocation for Theme Variable Injection:
Invoking.inject-FluentEditor-vars();inside the editor wrapper applies the theming variables fromvars.lessto this component. Please verify that the mixin is properly defined invars.lessand that its styles integrate well with the existing layout without causing side effects.
18-27: SVG Icon Styling Enhancements:
The newsvgblock within.ql-toolbar.ql-snowsets a consistent font size for icons and adjusts the fill color forpathelements. The hover/focus states now use CSS variables (--tv-FluentEditor-select-icon-color-hover) to enable dynamic theming. This change enhances maintainability and theme consistency.
64-89: Enhanced Picker Label Styling:
Within the nested.ql-picker-labelblock (inside picker-related elements), the text color is now set viavar(--tv-FluentEditor-selected-text-color). Additionally, the hover state is adjusted usingvar(--tv-FluentEditor-select-border-color-hover), and the::afterpseudo-element is configured to create a chevron effect with properties (width, height, border, transform) driven by CSS variables. This approach increases flexibility but ensure that the defined variable values offer sufficient contrast and adhere to accessibility guidelines.
92-95: Picker Label Hover State Updates:
The hover style for.ql-picker-labelnow utilizes the variables--tv-FluentEditor-select-border-color-hoverand--tv-FluentEditor-select-text-colorfor border and text color, respectively. This ensures a cohesive theme across normal and interactive states.
131-133: Picker Options Styling with Theme Variables:
The.ql-picker-optionsblock has been updated to set thebackground-color,border-radius, andbox-shadowusing CSS variables. This change will facilitate easier theme adjustments. Please verify that these variables are defined invars.lesswith sensible default or fallback values if needed.
135-141: Picker Item Color Customization:
The styling for.ql-picker-itemnow leverages theme variables for text color (--tv-FluentEditor-select-text-color) and updates its hover state with a background color (--tv-FluentEditor-select-bg-color-hover) and active text color (--tv-FluentEditor-select-active-text-color). This update aligns the item styles with the overall theme changes.
James-9696
changed the title
James-9696
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
examples/sites/demos/pc/app/fluent-editor/image-upload.spec.ts (1)
1-7: Good implementation of image upload test.The test is well-structured and follows the Playwright testing pattern. It correctly sets up error handling and navigates to the appropriate page before performing the click operation.
However, consider enhancing this test with additional assertions to verify what happens after clicking the image upload button (e.g., checking if an upload dialog appears or if a placeholder is shown).
examples/sites/demos/pc/app/fluent-editor/options.spec.ts (1)
1-9: Well-implemented test for editor configuration.The test correctly verifies the presence of the expected number of toolbar buttons (14) in the Fluent Editor. The approach using locators and count verification is appropriate.
For better maintainability, consider adding a comment explaining why the count is expected to be 14, so if toolbar changes are made in the future, it's clear why this test might need to be updated.
examples/sites/demos/pc/app/fluent-editor/disabled.spec.ts (1)
1-9: Good verification of disabled state.This test effectively verifies that the editor enters the disabled state by checking for the presence of the
.ql-disabledclass on the editor element.Consider adding an additional assertion to verify some functional aspect of the disabled state, such as checking that input attempts are blocked when the editor is disabled.
examples/sites/demos/pc/app/fluent-editor/basic-usage.spec.ts (1)
1-15: Comprehensive basic usage test with proper formatting verification.This test thoroughly checks the core functionality of the Fluent Editor by:
- Filling the editor with text
- Selecting all text
- Applying bold formatting
- Verifying the CSS properties to confirm the formatting was applied correctly
The approach with multiple steps and a specific verification of the CSS property is excellent.
For even better coverage, consider adding tests for other common formatting operations like italic, underline, or different text alignments.
examples/sites/demos/pc/app/fluent-editor/data-switch.spec.ts (1)
1-15: Test implementation looks functional but could be improvedThe test structure is solid but has a few potential improvements:
- Consider adding descriptive comments explaining the purpose of the test, especially since the test title is in Chinese ("数据格式转换" = "Data Format Conversion")
- The selector strategy using complex nested locators might be brittle if the DOM structure changes
- The assertion checks a very specific CSS 'counter-set' value which seems like an implementation detail rather than a functional requirement
Consider refactoring with these improvements:
import { test, expect } from '@playwright/test' +// Test for verifying data format conversion functionality in the FluentEditor test('数据格式转换', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('fluent-editor#data-switch') const demo = page.locator('#data-switch') + // Find and interact with the editor content await page.locator('#data-switch').getByText('Hello FluentEditor!').click() await page.locator('#data-switch #editor div').filter({ hasText: 'Hello FluentEditor!' }).press('ControlOrMeta+a') await page.locator('#data-switch').getByLabel('clean').click() + + // Verify the editor is cleaned properly const cls = demo.locator('.ql-editor p') + // Check counter-set property (representing empty list state) await expect(cls).toHaveCSS( 'counter-set', 'list-7 0 list-0 0 list-9 0 list-2 0 list-6 0 list-4 0 list-5 0 list-3 0 list-1 0 list-8 0' ) + + // Consider adding a more functional assertion that verifies the editor content is empty + // await expect(demo.locator('.ql-editor').textContent()).toBe(''); })examples/sites/demos/pc/app/fluent-editor/before-editor-init.spec.ts (1)
1-21:❓ Verification inconclusive
Test validates color functionality but has potential theme compatibility issues
The test correctly validates that the "good" and "bad" buttons apply different text colors, but there are some concerns:
- The hardcoded RGB color values (green and red) might break if theme colors change, as suggested by the PR title "fix(fluent-editor): [fluent-editor] change theme"
- The test uses complex nested locators that could be fragile if the DOM structure changes
- There are no comments explaining what the initialization hook is supposed to do
Run this script to check if the color values used in the test match the current theme variables:
Consider these improvements:
import { test, expect } from '@playwright/test' +// Test for verifying initialization hooks in the FluentEditor +// This test validates that text formatting works with good/bad buttons test('初始化前的钩子', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('fluent-editor#before-editor-init') const demo = page.locator('#before-editor-init') + // Select text in the editor await page.locator('#before-editor-init').getByText('Hello FluentEditor!').click() await page .locator('#before-editor-init #editor div') .filter({ hasText: 'Hello FluentEditor!' }) .press('ControlOrMeta+a') + // Apply "good" formatting and verify text is green await demo.getByLabel('good').click() const cls = demo.locator('.ql-editor strong') - await expect(cls).toHaveCSS('color', 'rgb(92, 179, 0)') + // Consider using a more theme-independent verification method + await expect(cls).toHaveCSS('color', 'rgb(92, 179, 0)') // Success color + // Re-select text await page .locator('#before-editor-init #editor div') .filter({ hasText: 'Hello FluentEditor!' }) .press('ControlOrMeta+a') + // Apply "bad" formatting and verify text is red await demo.getByLabel('bad').click() - await expect(cls).toHaveCSS('color', 'rgb(242, 48, 48)') + // Consider using a more theme-independent verification method + await expect(cls).toHaveCSS('color', 'rgb(242, 48, 48)') // Error color })
🏁 Script executed:
#!/bin/bash # Search for color variables in the fluent-editor theme files echo "Searching for theme color variables in fluent-editor theme files:" rg --color always -i -A 1 -B 1 'color.*rgb\(92, 179, 0\)' --glob '*.less' packages/theme/src/ rg --color always -i -A 1 -B 1 'color.*rgb\(242, 48, 48\)' --glob '*.less' packages/theme/src/Length of output: 333
Action Required: Review Test Fragility Regarding Hardcoded Colors and Locator Complexity
The current test properly validates that the "good" and "bad" buttons apply distinct text colors, but the following concerns remain:
- The hardcoded RGB values (green:
rgb(92, 179, 0)and red:rgb(242, 48, 48)) lack a clear link to theme variables. Our automated search did not locate these color definitions in the expected theme files, so please manually verify whether these colors are indeed fixed or if they rely on theme variables that could change.- The use of nested locators might be fragile if the DOM structure changes in future revisions.
- The test is missing comments to explain the purpose of the initialization hook; adding a brief comment would improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/sites/demos/pc/app/fluent-editor/basic-usage.spec.ts(1 hunks)examples/sites/demos/pc/app/fluent-editor/before-editor-init.spec.ts(1 hunks)examples/sites/demos/pc/app/fluent-editor/data-switch.spec.ts(1 hunks)examples/sites/demos/pc/app/fluent-editor/disabled.spec.ts(1 hunks)examples/sites/demos/pc/app/fluent-editor/image-upload.spec.ts(1 hunks)examples/sites/demos/pc/app/fluent-editor/options.spec.ts(1 hunks)
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Style
Tests