Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: optimize bind-i18n styles #925

Open
wants to merge 5 commits into
base: refactor/develop
Choose a base branch
from

Conversation

gene9831
Copy link
Collaborator

@gene9831 gene9831 commented Dec 5, 2024

English | 简体中文

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Built its own designer, fully self-validated

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe: Styles

Background and solution

优化属性面板绑定国际化UI

What is the current behavior?

Issue Number: N/A

What is the new behavior?

效果图

  1. 选择不带参数的国际化文案
    {46C028F1-6827-494D-8447-9D43AAA1619B}

  2. 选择带参数的国际化文案
    {CC555796-5B80-465D-82ED-6B65B4AB4541}

  3. 添加国际化并关联
    {49B390A8-7946-46F6-A020-E328D921C977}

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

遗留问题:在 tiny-select 组件的下拉面板中,固定了面板宽度后,如果某个选项的文本长度超长,会变成可横向滚动的面板(见下图1)。在 tiny-vue 最新版中,会自动截去多余文本,并且悬停可以显示详情(见下图2)。建议升级 tiny-vue 版本来解决这个问题,否则需要侵入式修改部分style

图1:下拉面板可横向滚动
image

图2:tiny-vue 最新版选择器
image

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Updated placeholder text for the language selection dropdown.
    • Added a title to the popover component for improved user guidance.
  • Improvements

    • Enhanced layout and styling for buttons and dropdowns, improving visual consistency.
    • Increased width of the popover for better content display.
    • Introduced new styles for button alignment and spacing.
  • Bug Fixes

    • Adjusted button types for consistency across the interface.

Copy link
Contributor

coderabbitai bot commented Dec 5, 2024

Walkthrough

The changes in this pull request involve modifications to the BindI18n.vue and I18nInput.vue components. Key updates include adjustments to the template structure, styling enhancements, and minor logic alterations. The tiny-select component's placeholder text was changed, and new CSS classes were introduced for improved styling. The I18nInput.vue component saw an increase in the width of the tiny-popover, along with structural and styling updates to enhance visual presentation. Overall, these changes aim to improve layout and consistency while maintaining core functionalities.

Changes

File Path Change Summary
packages/common/component/BindI18n.vue - Updated placeholder text for tiny-select component.
- Added new property is-drop-inherit-width to tiny-select.
- Modified tiny-option label to concatenate item.key with item[currentLang].
- Reorganized buttons into a div with class bottom-buttons and standardized button types.
- Removed certain margin properties and added new styles for button alignment.
packages/common/component/I18nInput.vue - Increased width of tiny-popover from 260 to 340 pixels.
- Changed popper class from "lowcode" to "i18n-input-popover".
- Added title element <h3 class="title">绑定国际化</h3> to popover.
- Adjusted .popover-content class and modified .icon-close class for better positioning.

Possibly related PRs

  • fix: improve the style priority of overwriting tinyvue #848: This PR modifies the CSS class definitions for various components, including .tiny-select, which is relevant to the changes made in the BindI18n.vue component regarding the tiny-select component's styling.
  • fix: modify button padding #894: This PR includes modifications to the .tiny-button and .tiny-select-dropdown classes, which are related to the button type changes and dropdown styling adjustments made in the BindI18n.vue component.

Suggested reviewers

  • chilingling
  • hexqi

🐰 In the meadow where we play,
Changes come to brighten the day.
With buttons aligned and styles anew,
Our components shine, a vibrant view!
Hopping forward, we embrace the light,
In code we trust, everything feels right! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c3c6a39 and 134dd0c.

📒 Files selected for processing (1)
  • packages/common/component/BindI18n.vue (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/common/component/BindI18n.vue

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added bug Something isn't working refactor-main refactor/develop branch feature labels Dec 5, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
packages/common/component/I18nInput.vue (1)

144-148: Consider scoping the popover styles.

Using global styles (without scoped) could lead to unintended style conflicts. Consider moving these styles into the scoped section or using a more specific selector.

-<style>
+<style lang="less" scoped>
-.i18n-input-popover {
+:deep(.i18n-input-popover) {
   padding: 20px !important;
 }
 </style>
packages/common/component/BindI18n.vue (2)

182-183: Consider using CSS custom properties for consistent spacing

While the styling changes improve layout, consider using CSS custom properties (variables) for consistent spacing values across the component.

<style lang="less" scoped>
+:root {
+  --spacing-base: 8px;
+  --spacing-medium: 16px;
+}
.tiny-button.tiny-button.tiny-button--default {
  margin: 0;
}
.label {
-  margin: 16px 0;
+  margin: var(--spacing-medium) 0;
  font-size: 12px;
  line-height: 18px;
}

Also applies to: 204-210


228-231: Consider scoping the dropdown styles

While the max-width constraint is reasonable, consider scoping these styles to avoid potential conflicts with other components using the same class.

-<style>
+<style lang="less" scoped>
-.select-i18n-drop {
+:deep(.select-i18n-drop) {
  max-width: 300px;
}
</style>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dee65eb and d6a7e4a.

📒 Files selected for processing (2)
  • packages/common/component/BindI18n.vue (6 hunks)
  • packages/common/component/I18nInput.vue (2 hunks)
🔇 Additional comments (5)
packages/common/component/I18nInput.vue (2)

128-139: LGTM! Well-structured layout.

The positioning and spacing provide a clean, organized layout for the popover content.


7-7: LGTM! Verify the increased popover width.

The increased width and semantic class name improve the UI. However, please verify that the wider popover (340px) doesn't cause layout issues in narrow viewports.

Also applies to: 10-10

✅ Verification successful

Based on the search results, I can see that the application has proper responsive handling with various media queries and width constraints. The I18nInput component's popover width of 340px is well within reasonable limits, considering:

  1. The codebase shows several components with similar or larger widths:

    • Dialog widths: 700-760px
    • Popovers/panels: 300-500px
    • Form elements: 300-400px
  2. The application has responsive breakpoints at common viewport sizes (1280px, 1400px, 1600px, 1920px) with appropriate styling adjustments.

  3. The base minimum width of the application is 1280px (from theme variables), which provides ample space for a 340px popover.

The popover width increase is safe and follows the application's UI patterns.

The wider popover aligns with other similar UI elements in the codebase and won't cause layout issues given the application's responsive design system and minimum viewport width requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any responsive handling or width constraints in the codebase
rg -g '*.{vue,less,css}' '(max-width|min-width|@media|responsive)'

Length of output: 22146

packages/common/component/BindI18n.vue (3)

7-7: LGTM! Improved select component UX

The changes enhance usability through a more descriptive placeholder and controlled dropdown width.

Also applies to: 10-10


28-33: LGTM! Improved button organization and hierarchy

The changes enhance the UI by:

  • Using proper button hierarchy (primary/secondary)
  • Implementing consistent button spacing
  • Improving layout with flex container

48-50: LGTM! Consistent button styling

The changes maintain visual consistency with other button groups in the component.

packages/common/component/I18nInput.vue Show resolved Hide resolved
packages/common/component/BindI18n.vue Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
packages/common/component/BindI18n.vue (2)

17-29: Consider adding ARIA role for better accessibility

While using div.label for block-level layout is justified (per previous discussion), consider adding an ARIA role for better accessibility:

-<div class="label">国际化参数配置</div>
+<div class="label" role="heading" aria-level="2">国际化参数配置</div>

The button layout improvements with flex and proper spacing look good!


Line range hint 178-221: Consider using CSS variables for consistent spacing

The layout improvements look good! Consider extracting common values into CSS variables for better maintainability:

 .bottom-buttons {
   display: flex;
   justify-content: flex-end;
-  margin-top: 16px;
-  gap: 8px;
+  margin-top: var(--tiny-spacing-4x, 16px);
+  gap: var(--tiny-spacing-2x, 8px);
 }

This would help maintain consistent spacing across the application.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9435b50 and c3c6a39.

📒 Files selected for processing (2)
  • packages/common/component/BindI18n.vue (5 hunks)
  • packages/common/component/I18nInput.vue (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/common/component/I18nInput.vue
🔇 Additional comments (3)
packages/common/component/BindI18n.vue (3)

44-46: LGTM! Consistent button styling

The button layout maintains visual consistency with the main section and establishes clear action hierarchy.


64-64: LGTM! Fixed component registration

Properly registering the TinyInput component that was already in use.


7-13: LGTM! Template changes improve UX

The changes enhance usability through:

  • More descriptive placeholder text
  • Consistent dropdown width with is-drop-inherit-width
  • Improved option labels showing both key and translated text
✅ Verification successful

Based on the search results, I can now generate the final response since I have enough context about the is-drop-inherit-width prop usage:

The is-drop-inherit-width prop is correctly implemented and used

The prop is consistently used across the codebase:

  • In BindI18n.vue for language selection dropdown
  • In SelectConfigurator.vue as a configurable option
  • The prop is properly supported by the tiny-select component's CSS implementation as evidenced by the dropdown styling rules
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if is-drop-inherit-width prop is documented and consistently used
ast-grep --pattern 'tiny-select $$$is-drop-inherit-width$$$'

Length of output: 62


Script:

#!/bin/bash
# Let's try a different approach to verify the prop usage and documentation
# First, let's search for any documentation or usage of is-drop-inherit-width
rg "is-drop-inherit-width" -A 2 -B 2

# Also search for tiny-select usage patterns to understand the context
rg "tiny-select" -A 2 -B 2

Length of output: 64769

hexqi
hexqi previously approved these changes Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor-main refactor/develop branch feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants