fix(refactor/styles): fix property panel styles and common styles #887
Conversation
WalkthroughThe pull request introduces a series of updates across multiple Vue components, focusing on styling, layout adjustments, and minor logic refinements. Key changes include the introduction of CSS variables for improved spacing and styling consistency, enhancements to error handling UI, and structural modifications to templates for better responsiveness. Additionally, several components have had their visual elements updated to align with a new design system while maintaining core functionalities and logic. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (15)
packages/common/component/MetaChildItem.vue (1)
96-96: Consider using CSS variable for margin-bottom.
For better consistency with other components in the codebase, consider using the CSS variable --te-common-vertical-item-spacing-normal instead of the hard-coded value.
- margin-bottom: 16px;
+ margin-bottom: var(--te-common-vertical-item-spacing-normal);packages/settings/events/src/components/BindEventsDialogContent.vue (1)
Line range hint 65-91: Consider optimizing validation rules definition.
The validation rules array is currently recreated on every change event. Consider moving it outside the function to improve performance.
Here's a suggested optimization:
+ const validRules = [
+ { validator: validMethodNameEmpty, tip: METHOD_TIPS_MAP.empty },
+ { validator: validMethodNameExist, tip: METHOD_TIPS_MAP.exist },
+ { validator: invalidMethodName, tip: METHOD_TIPS_MAP.ruleInvalid }
+ ]
const change = (value) => {
- const validRules = [
- { validator: validMethodNameEmpty, tip: METHOD_TIPS_MAP.empty },
- { validator: validMethodNameExist, tip: METHOD_TIPS_MAP.exist },
- { validator: invalidMethodName, tip: METHOD_TIPS_MAP.ruleInvalid }
- ]
for (let i = 0; i < validRules.length; i++) {
const rule = validRules[i]
if (rule.validator(value)) {
context.tipError = true
context.tip = rule.tip
return
}
}
context.tipError = false
context.tip = ''
}packages/theme/base/src/page/base-config-page.less (1)
135-139: Consider using consistent padding values.
The base styles look good and follow the project's theming conventions with CSS variables. However, the asymmetric padding (0px 8px 12px) might affect content alignment.
Consider using consistent padding for better visual balance:
- padding: 0px 8px 12px;
+ padding: 8px;packages/settings/events/src/components/BindEventsDialog.vue (3)
Line range hint 89-102: Enhance error handling in getExtraParams.
The function could provide more specific error messages to help developers understand what went wrong during parameter parsing.
Consider this improvement:
const getExtraParams = () => {
let extraParams = ''
if (state.enableExtraParams) {
try {
extraParams = JSON.parse(state.editorContent)
state.isValidParams = Array.isArray(extraParams)
+ if (!state.isValidParams) {
+ state.tip = 'Extra parameters must be an array'
+ }
} catch (error) {
state.isValidParams = false
+ state.tip = `Invalid JSON format: ${error.message}`
}
}
return extraParams
}Line range hint 104-156: Consider splitting the confirm function for better maintainability.
The function handles multiple responsibilities: parameter validation, method binding, saving, and plugin activation. Consider breaking it down into smaller, focused functions.
Here's a suggested refactor:
+const prepareParameters = (state) => {
+ const params = 'event'
+ const extraParams = getExtraParams()
+
+ if (!state.isValidParams) {
+ return null
+ }
+
+ if (!extraParams) {
+ return { params, formatParams: params, extraParams: null }
+ }
+
+ return {
+ params: extraParams.join(','),
+ formatParams: getFormatParams(extraParams),
+ extraParams
+ }
+}
+const saveMethodWithBody = async (state, params) => {
+ const functionBody = getFunctionBody()
+ const { name } = state.bindMethodInfo
+ const method = {
+ name,
+ content: state.enableExtraParams
+ ? `function ${name}(eventArgs,${params.formatParams}) ${functionBody}`
+ : `function ${name}(${params.formatParams}) ${functionBody}`
+ }
+
+ const { beforeSaveMethod } = getOptions(meta.id)
+ if (typeof beforeSaveMethod === 'function') {
+ await beforeSaveMethod(method, state.bindMethodInfo)
+ }
+
+ saveMethod?.(method)
+}
const confirm = async () => {
if (state.tipError) {
return
}
- let params = 'event'
- let extraParams = getExtraParams()
- let formatParams = params
-
- if (!state.isValidParams) {
- return
- }
-
- if (extraParams) {
- params = extraParams.join(',')
- formatParams = getFormatParams(extraParams)
- }
+ const params = prepareParameters(state)
+ if (!params) return
bindMethod({ ...state.bindMethodInfo, params, extra: extraParams })
-
- // 需要在bindMethod之后
- const functionBody = getFunctionBody()
- const { name } = state.bindMethodInfo
- const method = {
- name,
- content: state.enableExtraParams
- ? `function ${name}(eventArgs,${formatParams}) ${functionBody}`
- : `function ${name}(${formatParams}) ${functionBody}`
- }
- const { beforeSaveMethod } = getOptions(meta.id)
-
- if (typeof beforeSaveMethod === 'function') {
- await beforeSaveMethod(method, state.bindMethodInfo)
- }
-
- saveMethod?.(method)
+ await saveMethodWithBody(state, params)
activePagePlugin()
close()
}Line range hint 1-156: Consider adding TypeScript for better type safety.
The component handles complex event binding and parameter validation. Adding TypeScript would help catch potential type-related issues early and improve maintainability.
Consider:
- Converting the file to TypeScript (
.vue→.tsx) - Adding interfaces for event binding and method information
- Adding proper type annotations for function parameters and return values
Would you like me to provide an example of how this could be implemented?
packages/configurator/src/js-slot-configurator/JsSlotConfigurator.vue (1)
Line range hint 63-69: Consider making componentsMap more maintainable and extensible.
The current hardcoded componentsMap implementation has several limitations:
- Adding new components requires code changes
- Content is not internationalized
- Limited extensibility
Consider:
- Moving component definitions to a configuration file
- Adding i18n support for tooltip content
- Implementing a registration mechanism for new components
Would you like assistance in implementing these improvements?
packages/common/component/MetaCodeEditor.vue (2)
272-277: Consider using CSS variables for spacing and font sizes.
The styles are well-structured, but could be more maintainable by using CSS variables for spacing and font sizes, consistent with the project's design system.
Here's the suggested improvement:
.edit-btn-icon {
- font-size: 14px;
- margin-right: 4px;
+ font-size: var(--te-font-size-small, 14px);
+ margin-right: var(--te-spacing-small, 4px);
vertical-align: text-top;
color: var(--te-common-icon-secondary);
}Line range hint 1-394: Consider splitting the component for better maintainability.
The component handles multiple responsibilities (editing, formatting, validation). Consider:
- Extract the JSON parsing and formatting logic into a separate utility
- Create a separate component for the dialog box to reduce complexity
- Implement a more robust error handling strategy with specific error types
Example structure:
// utils/json.ts
export class JSONParseError extends Error {
constructor(message: string, public readonly originalError: Error) {
super(message)
}
}
export const parseJSON = (content: string) => {
try {
return JSON.parse(content)
} catch (error) {
throw new JSONParseError('Invalid JSON format', error)
}
}
// components/CodeEditorDialog.vue
// Split the dialog box into its own componentpackages/settings/events/src/components/BindEvents.vue (2)
Line range hint 368-374: Consider enhancing accessibility for disabled state.
The styling changes look good, but consider adding ARIA attributes to improve accessibility for screen readers.
Add aria-disabled="true" to disabled list items:
<li
v-for="(event, name) in state.componentEvents"
:key="name"
:class="['bind-event-list-item', { 'bind-event-list-item-notallow': state.bindActions[name] }]"
+ :aria-disabled="state.bindActions[name]"
@click="openActionDialog({ eventName: name }, true)"
>Line range hint 1-391: Consider architectural improvements for maintainability.
While the current changes are good, here are some suggestions for future improvements:
- Extract event handling logic into a dedicated composable (e.g.,
useEventBinding) to improve reusability and testing. - Consider implementing proper dark mode support for the color variables.
- Reduce coupling with
pageStateby introducing a dedicated events store or service.
packages/settings/styles/src/components/background/BackgroundGroup.vue (3)
Line range hint 17-35: Enhance keyboard accessibility for background image management.
The background image list items support drag and drop but lack keyboard navigation and operations. Consider adding keyboard support for better accessibility.
Add keyboard support for:
- Arrow keys for navigation
- Space/Enter for selection
- Ctrl + Arrow keys for reordering
Example implementation for keyboard navigation:
<div
v-for="(item, index) in state.backgroundImageList"
:key="index"
:draggable="true"
+ tabindex="0"
+ role="listitem"
+ @keydown="handleKeyDown($event, item, index)"
:class="[
'image-list-item',
item.forbiddenChildrenPointerEvents,
item.dragStart ?? '',
item.invisible ? 'image-list-item-invisible' : ''
]"Line range hint 449-493: Improve drag and drop implementation robustness.
The current drag and drop implementation has several potential issues:
- Module-level variables (
dragFrom,dragTo) could cause issues with multiple component instances - The
setTimeoutinhandleDragstartcould cause visual glitches - Race conditions could occur during rapid drag operations
Consider refactoring the drag and drop implementation:
-let dragFrom = null
-let dragTo = null
-let dragToIndex = null
export default {
setup(props, { emit }) {
+ const dragState = reactive({
+ dragFrom: null,
+ dragTo: null,
+ dragToIndex: null
+ })
const handleDragstart = (e, item) => {
- dragFrom = e.target
- setTimeout(() => {
- item.dragStart = 'drag-start'
- }, 0)
+ dragState.dragFrom = e.target
+ item.dragStart = 'drag-start'
}
const handleDragenter = (e, item, index) => {
if (!e.target.hasAttribute('draggable')) {
return
}
- if (e.target === dragFrom) {
+ if (e.target === dragState.dragFrom) {
return
}
- dragTo = e.target
- dragToIndex = index
+ dragState.dragTo = e.target
+ dragState.dragToIndex = index
item.draging = true
item.forbiddenChildrenPointerEvents = 'forbidden-children-pointer-events'
}Line range hint 394-413: Add error handling for background image operations.
The background image operations lack proper error handling and URL validation. This could lead to broken images or security issues.
Consider adding validation and error handling:
const openBackgroundImageModal = (event, { isAdd, index }) => {
if (isAdd) {
+ const validateImageUrl = (url) => {
+ try {
+ new URL(url)
+ return true
+ } catch {
+ return false
+ }
+ }
+
+ const defaultImageUrl = 'img/bgcModal.png'
const styleObj = {
- [BACKGROUND_PROPERTY.BackgroundImage]: 'url(img/bgcModal.png)',
+ [BACKGROUND_PROPERTY.BackgroundImage]: validateImageUrl(defaultImageUrl)
+ ? `url(${defaultImageUrl})`
+ : '',
[BACKGROUND_PROPERTY.BackgroundPosition]: '0px 0px',
[BACKGROUND_PROPERTY.BackgroundSize]: 'auto'
}packages/common/component/ConfigItem.vue (1)
521-521: LGTM! Good use of CSS variable for consistent spacing.
The change from a hardcoded padding value to the --te-common-vertical-item-spacing-normal CSS variable aligns with best practices for maintaining consistent spacing across components.
Consider documenting these common spacing variables in a central style guide to help other developers understand the available spacing options and their intended usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/design-core/assets/empty-action.svgis excluded by!**/*.svgpackages/design-core/assets/page-schema.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
- packages/common/component/ConfigItem.vue (1 hunks)
- packages/common/component/MetaChildItem.vue (1 hunks)
- packages/common/component/MetaCodeEditor.vue (2 hunks)
- packages/configurator/src/js-slot-configurator/JsSlotConfigurator.vue (3 hunks)
- packages/configurator/src/slider-configurator/SliderConfigurator.vue (1 hunks)
- packages/configurator/src/slot-configurator/SlotConfigurator.vue (1 hunks)
- packages/settings/events/src/components/AdvanceConfig.vue (1 hunks)
- packages/settings/events/src/components/BindEvents.vue (3 hunks)
- packages/settings/events/src/components/BindEventsDialog.vue (1 hunks)
- packages/settings/events/src/components/BindEventsDialogContent.vue (1 hunks)
- packages/settings/events/src/components/BindEventsDialogSidebar.vue (1 hunks)
- packages/settings/styles/src/components/background/BackgroundGroup.vue (1 hunks)
- packages/settings/styles/src/components/border/BorderGroup.vue (1 hunks)
- packages/settings/styles/src/components/effects/EffectGroup.vue (1 hunks)
- packages/settings/styles/src/components/size/SizeGroup.vue (1 hunks)
- packages/settings/styles/src/components/typography/TypographyGroup.vue (3 hunks)
- packages/theme/base/src/base.less (1 hunks)
- packages/theme/base/src/common.less (1 hunks)
- packages/theme/base/src/component-common.less (3 hunks)
- packages/theme/base/src/page/base-config-page.less (1 hunks)
- packages/theme/dark/variable.less (1 hunks)
- packages/theme/light/variable.less (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/configurator/src/slot-configurator/SlotConfigurator.vue
- packages/settings/events/src/components/AdvanceConfig.vue
- packages/settings/events/src/components/BindEventsDialogSidebar.vue
- packages/settings/styles/src/components/border/BorderGroup.vue
- packages/settings/styles/src/components/effects/EffectGroup.vue
- packages/settings/styles/src/components/size/SizeGroup.vue
🔇 Additional comments (19)
packages/common/component/MetaChildItem.vue (1)
108-108: LGTM! Good use of CSS variable for spacing.
The use of --te-common-vertical-item-spacing-normal aligns well with the standardization of spacing across components.
packages/configurator/src/slider-configurator/SliderConfigurator.vue (3)
Line range hint 1-24: LGTM! Template structure is well-organized.
The component effectively combines a range slider with a number configurator, providing a good user experience for value input.
136-136: Verify slider track visibility with transparent background.
Setting background-color: transparent might affect the visibility of the slider track. Consider using a subtle background color or ensuring the track is visible through other means (like the .tiny-slider__range class).
Line range hint 1-157: Verify slider behavior across different browsers.
Since the styling changes affect the appearance of the range input, please ensure to test the component's behavior and appearance across different browsers, particularly:
- Chrome/Edge (Webkit/Blink)
- Firefox (Gecko)
- Safari (Webkit)
packages/settings/events/src/components/BindEventsDialogContent.vue (2)
149-149: LGTM! Good use of CSS variable for consistent spacing.
The change from hardcoded margin value to --te-common-vertical-item-spacing-normal improves maintainability and ensures consistent spacing across components.
Line range hint 3-3: LGTM! Well-implemented error handling UI.
The error state management through CSS classes and variables provides clear visual feedback while maintaining consistency with the design system.
Also applies to: 156-166
packages/theme/base/src/common.less (1)
50-52: LGTM! Well-documented spacing variables.
The new spacing section with its variables is well-organized and clearly documented. The comments effectively explain the intended usage of each spacing variable.
packages/settings/events/src/components/BindEventsDialog.vue (1)
250-251: LGTM! Good use of design system variables.
The replacement of hardcoded spacing values with CSS variables (--te-common-vertical-item-spacing-normal) improves consistency and maintainability.
packages/configurator/src/js-slot-configurator/JsSlotConfigurator.vue (1)
196-196: LGTM: Style improvements enhance visual consistency.
The alignment and spacing changes improve the visual presentation of the slot configurator:
- Centered alignment of switch elements
- Consistent icon spacing
Also applies to: 220-220
packages/settings/events/src/components/BindEvents.vue (2)
316-316: LGTM: Spacing adjustment aligns with design system.
The increased margin provides better visual separation between action buttons.
347-347: LGTM: Good use of design system variables.
Using --te-common-vertical-item-spacing-normal improves consistency and maintainability across themes.
packages/theme/light/variable.less (1)
330-332: LGTM! Verify referenced variables exist.
Good refactoring to use CSS custom properties instead of hardcoded values. This improves maintainability and consistency across themes.
Let's verify that the referenced variables exist in the codebase:
Consider adding comments documenting the color system variables (--te-common-bg-* and --te-base-*) and their intended usage to help other developers understand the theming structure.
✅ Verification successful
Variables are properly defined and used consistently across themes
The referenced CSS variables are well-defined in the theme system:
--te-common-bg-primary-checkedis defined inbase/src/common.lessand maps to--te-base-blue-6--te-base-blue-25is defined inbase/src/base.lesswith valuergba(20, 118, 255, 0.1)- Both variables are used consistently across light and dark themes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new CSS variables are defined
# Expected: Find definitions of --te-common-bg-primary-checked and --te-base-blue-25
# Search for the variable definitions
echo "Searching for --te-common-bg-primary-checked..."
rg --type css "--te-common-bg-primary-checked"
echo "Searching for --te-base-blue-25..."
rg --type css "--te-base-blue-25"
Length of output: 1117
Script:
#!/bin/bash
# Description: Check if the new CSS variables are defined
# Expected: Find definitions of --te-common-bg-primary-checked and --te-base-blue-25
# Search for the variable definitions with proper escaping
echo "Searching for --te-common-bg-primary-checked..."
rg "\-\-te\-common\-bg\-primary\-checked" --type css
echo "Searching for --te-base-blue-25..."
rg "\-\-te\-base\-blue\-25" --type css
# Also search in less files as the original file is a .less file
echo "Searching in .less files..."
rg "\-\-te\-(common\-bg\-primary\-checked|base\-blue\-25)" -g "*.less"
Length of output: 1935
packages/theme/base/src/base.less (1)
285-285: LGTM on variable naming and value.
The new variable follows the established naming pattern and its value aligns well with the existing color scheme. The semi-transparent blue color (10% opacity) is consistent with other similar variables in the codebase.
packages/theme/dark/variable.less (1)
353-353: LGTM! Verify referenced variable exists.
The change from a hardcoded color to a variable reference improves theme consistency and maintainability.
Let's verify that the referenced variable is defined:
✅ Verification successful
Variable reference is properly defined and used consistently
The referenced variable --te-common-bg-primary-checked is defined in packages/theme/base/src/common.less and set to var(--te-base-blue-6) with a blue color fallback. It's also consistently used across multiple theme files for similar styling purposes, making this change align well with the existing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of --te-common-bg-primary-checked
rg --type less "te-common-bg-primary-checked"
Length of output: 1385
packages/settings/styles/src/components/typography/TypographyGroup.vue (3)
476-476: LGTM: Good use of design system spacing variable.
The change from a hardcoded margin value to --te-common-vertical-item-spacing-normal improves maintainability and consistency across the design system.
491-491: 🛠️ Refactor suggestion
Verify vertical alignment with reduced line-height.
The line-height has been significantly reduced from 26px to 16px. While this creates a more compact layout, please verify that:
- Text remains vertically centered within its container
- This aligns with the design system's vertical rhythm
- No text truncation occurs with different font sizes
Consider using a relative unit or a design system variable instead of a fixed pixel value:
- line-height: 16px;
+ line-height: var(--te-typography-label-line-height, 16px);500-500: 🛠️ Refactor suggestion
Verify touch target size after padding reduction.
The vertical padding has been removed while maintaining horizontal padding. Please verify that:
- The touch target size remains adequate for mobile/touch devices (recommended minimum is 44x44px)
- The icons remain vertically centered within their containers
Consider using design system variables for consistent padding:
- padding: 0 5px;
+ padding: 0 var(--te-typography-content-item-padding-horizontal, 5px);packages/common/component/ConfigItem.vue (1)
521-521: 🛠️ Refactor suggestion
Consider enhancing error state accessibility.
While the error state styling is well-implemented, consider adding ARIA attributes to improve accessibility:
Add these attributes to the error container:
- <div v-if="showErrorPopup" class="error-tips-container">
+ <div
+ v-if="showErrorPopup"
+ class="error-tips-container"
+ role="alert"
+ aria-live="polite"
+ >Likely invalid or redundant comment.
packages/theme/base/src/component-common.less (1)
117-117: LGTM! Good use of CSS variable for consistent spacing.
The change to use --te-common-vertical-item-spacing-normal improves maintainability and ensures consistent vertical spacing across components.
packages/configurator/src/slider-configurator/SliderConfigurator.vue
Outdated
Show resolved
Hide resolved
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
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
Release Notes
New Features
Improvements
Style Updates
Bug Fixes