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

feat(locales): [locales] Optimize the style and demo #2475

Closed
wants to merge 3 commits into from

Conversation

Youyou-smiles
Copy link
Collaborator

@Youyou-smiles Youyou-smiles commented Oct 31, 2024

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)

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:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

Release Notes

  • New Features

    • Added multiple instances of the <tiny-amount> component with varying sizes in the size-composition-api.vue and size.vue files for enhanced user input options.
    • Introduced notifications for drawer open/close events in the use-through-method.vue and use-through-method-composition-api.vue components to improve user feedback.
  • Bug Fixes

    • Updated expected values in tests to reflect changes in naming conventions for better accuracy.
  • Style

    • Enhanced styling flexibility by implementing CSS variables in various components, including area and locale styles.
  • Documentation

    • Updated locale handling to use standardized identifiers for improved consistency across the application.
    • Updated the version of the @opentiny/vue-locales package to ensure users have the latest features and fixes.

Copy link

coderabbitai bot commented Oct 31, 2024

Walkthrough

The changes in this pull request involve modifications across multiple Vue components and stylesheets. Key updates include the addition of multiple <tiny-amount> components with varying sizes, adjustments to locale handling methods, and enhancements to user notifications in drawer components. Additionally, new LESS files and variables are introduced to improve styling flexibility. The overall structure of the components remains intact, with a focus on enhancing user experience and maintainability through these updates.

Changes

File Path Change Summary
examples/sites/demos/pc/app/amount/size-composition-api.vue Added three <tiny-amount> components with sizes "small", "mini", maintaining existing bindings.
examples/sites/demos/pc/app/amount/size.vue Added three <tiny-amount> components with sizes "small", "mini", and one without size, maintaining bindings.
examples/sites/demos/pc/app/area/custom-service-composition-api.vue Modified getJCR function to change names in jcr array.
examples/sites/demos/pc/app/area/custom-service.spec.ts Updated expected value in a test case to reflect name change.
examples/sites/demos/pc/app/area/custom-service.vue Modified getJCR function to change names in jcr array.
examples/sites/demos/pc/app/drawer/use-through-method-composition-api.vue Added Notify import, replaced console logs with notifications, modified headerRight slot styles.
examples/sites/demos/pc/app/drawer/use-through-method.vue Added Notify import, added closeDrawer method, updated event handlers for notifications, replaced Button with TinyButton.
examples/sites/demos/pc/app/locales/custom-service-composition-api.vue Updated locale handling methods to return new format for locale identifiers.
examples/sites/demos/pc/app/locales/custom-service.spec.ts Updated expected value in a test case to reflect new locale format.
examples/sites/demos/pc/app/locales/custom-service.vue Updated locale handling methods to return new format for locale identifiers.
packages/theme/src/area/index.less Added import for vars.less, updated class definitions and margins to use CSS variables.
packages/theme/src/area/vars.less Introduced new LESS file defining custom properties for area component styling.
packages/theme/src/locales/index.less Added import for vars.less, updated classes to use locale-specific variables.
packages/theme/src/locales/vars.less Introduced new LESS function defining locale-specific CSS custom properties.
packages/vue/src/locales/src/pc.vue Added classes for styling localized text and icons, updated icon imports.
packages/vue/src/locales/package.json Updated version from 3.19.0 to 3.19.1.

Possibly related PRs

Suggested reviewers

  • zzcr

Poem

🐰 In the garden where changes bloom,
Tiny amounts in colors loom.
Locale whispers, soft and clear,
Notifications ringing, bringing cheer!
With styles anew, we hop with glee,
A vibrant world for all to see! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dd6e2c5 and 8acf1c7.

📒 Files selected for processing (1)
  • packages/vue/src/locales/package.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/vue/src/locales/package.json

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 the enhancement New feature or request (功能增强) label Oct 31, 2024
Copy link

@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: 8

🧹 Outside diff range and nitpick comments (15)
examples/sites/demos/pc/app/amount/size-composition-api.vue (1)

4-4: Consider explicitly setting the default size for clarity.

The TinyAmount component without a size prop relies on the default size. While this works, explicitly setting the size as "default" or "medium" would make the demo more self-documenting.

-  <tiny-amount v-model="value" :date="date"></tiny-amount>
+  <tiny-amount size="medium" v-model="value" :date="date"></tiny-amount>
examples/sites/demos/pc/app/amount/size.vue (2)

3-3: Consider using CSS for spacing instead of <br /> tags.

Using <br /> tags for layout spacing is not recommended as it mixes presentational concerns with content. Consider using CSS margins or a flex/grid container instead.

-<tiny-amount size="medium" v-model="value" :date="date"></tiny-amount>
-<br />
-<tiny-amount v-model="value" :date="date"></tiny-amount>
-<br />
-<tiny-amount size="small" v-model="value" :date="date"></tiny-amount>
-<br />
-<tiny-amount size="mini" v-model="value" :date="date"></tiny-amount>
+<div class="amount-demo">
+  <tiny-amount size="medium" v-model="value" :date="date"></tiny-amount>
+  <tiny-amount v-model="value" :date="date"></tiny-amount>
+  <tiny-amount size="small" v-model="value" :date="date"></tiny-amount>
+  <tiny-amount size="mini" v-model="value" :date="date"></tiny-amount>
+</div>

<style scoped>
.amount-demo {
  display: flex;
  flex-direction: column;
  gap: 1rem;
}
</style>

Also applies to: 5-5, 7-7


1-8: Add descriptive labels for better accessibility and demo clarity.

Since this is a demo showcasing different size variants, consider adding labels or descriptions to help users understand the different sizes being demonstrated.

-<template>
-  <tiny-amount size="medium" v-model="value" :date="date"></tiny-amount>
-  <br />
-  <tiny-amount v-model="value" :date="date"></tiny-amount>
-  <br />
-  <tiny-amount size="small" v-model="value" :date="date"></tiny-amount>
-  <br />
-  <tiny-amount size="mini" v-model="value" :date="date"></tiny-amount>
+<template>
+  <div class="amount-demo">
+    <div class="amount-variant">
+      <span class="variant-label">Medium size:</span>
+      <tiny-amount size="medium" v-model="value" :date="date"></tiny-amount>
+    </div>
+    <div class="amount-variant">
+      <span class="variant-label">Default size:</span>
+      <tiny-amount v-model="value" :date="date"></tiny-amount>
+    </div>
+    <div class="amount-variant">
+      <span class="variant-label">Small size:</span>
+      <tiny-amount size="small" v-model="value" :date="date"></tiny-amount>
+    </div>
+    <div class="amount-variant">
+      <span class="variant-label">Mini size:</span>
+      <tiny-amount size="mini" v-model="value" :date="date"></tiny-amount>
+    </div>
+  </div>
</template>

<style scoped>
.amount-demo {
  display: flex;
  flex-direction: column;
  gap: 1rem;
}

.amount-variant {
  display: flex;
  align-items: center;
  gap: 1rem;
}

.variant-label {
  min-width: 100px;
  color: #666;
}
</style>
examples/sites/demos/pc/app/locales/custom-service-composition-api.vue (1)

Line range hint 22-26: Consider standardizing URL paths to match locale formats.

While the locale format has been standardized to use underscores (e.g., 'en_US'), the URL paths still use hyphens (e.g., 'en-US'). This inconsistency might cause confusion or maintenance issues.

Consider updating the URLs to maintain consistency:

-    return Promise.resolve(`${window.location.origin}/#/webenglish/en-US/component/locales/custom-service`)
+    return Promise.resolve(`${window.location.origin}/#/webenglish/en_US/component/locales/custom-service`)
-    return Promise.resolve(`${window.location.origin}/#/zh-CN/component/custom-service`)
+    return Promise.resolve(`${window.location.origin}/#/zh_CN/component/custom-service`)
examples/sites/demos/pc/app/locales/custom-service.vue (2)

22-22: Document the return type structure.

The method returns a Promise resolving to an array containing a single locale. Consider adding JSDoc to clarify this return type structure.

+    /**
+     * Gets the current locale
+     * @returns {Promise<string[]>} Promise resolving to an array containing the current locale
+     */
     getCurrentLocale() {
       return Promise.resolve(['zh_CN'])
     },

Line range hint 25-29: Improve URL handling maintainability.

The current implementation has several potential issues:

  1. Hardcoded URLs reduce reusability across different applications
  2. Inconsistent URL patterns between locales (/webenglish/en-US/ vs /zh-CN/)
  3. Tight coupling to specific routing structure

Consider refactoring to use configurable URL patterns:

+    // Add these as component props with defaults
+    baseUrl: {
+      type: String,
+      default: () => window.location.origin
+    },
+    urlPattern: {
+      type: String,
+      default: '/#/:locale/component/locales/custom-service'
+    },
+
     getChangeLocaleUrl(targetLocale) {
-      if (targetLocale === 'en_US') {
-        return Promise.resolve(`${window.location.origin}/#/webenglish/en-US/component/locales/custom-service`)
-      } else {
-        return Promise.resolve(`${window.location.origin}/#/zh-CN/component/custom-service`)
-      }
+      const path = this.urlPattern.replace(':locale', targetLocale)
+      return Promise.resolve(`${this.baseUrl}${path}`)
     }
packages/theme/src/locales/vars.less (3)

14-31: Consider translating comments to English for better maintainability.

The current Chinese comments could be translated to English to improve accessibility for all developers. Here's a suggested translation:

-  // 图标距离文字间距
+  // Icon spacing from text
-  // 图标尺寸
+  // Icon size
-  // 图标颜色
+  // Icon color
-  // 文字字号
+  // Text font size
-  // 文字颜色
+  // Text color
-  // 弹框间距
+  // Popup padding
-  // 弹框选项间距
+  // Popup item padding
-  // 弹框选项行高
+  // Popup item line height
-  // 弹框选项悬浮色
+  // Popup item hover background color

13-32: Document theme token dependencies.

The mixin uses several theme tokens (e.g., --tv-space-sm, --tv-color-text) as fallback values. Consider adding documentation about these required theme dependencies at the top of the mixin.

 .inject-Locales-vars() {
+  // Dependencies:
+  // - --tv-space-sm: Small spacing (4px)
+  // - --tv-space-md: Medium spacing (8px)
+  // - --tv-space-xl: Large spacing (16px)
+  // - --tv-font-size-md: Medium font size (14px)
+  // - --tv-color-text: Text color (#191919)
+  // - --tv-color-bg-hover: Hover background color (#f5f5f5)

13-32: Consider adding size variants for different viewport sizes.

The current implementation uses fixed values. Consider adding size variants for different viewport sizes to improve responsiveness.

Example approach:

.inject-Locales-vars() {
  @media (max-width: 768px) {
    --tv-Locales-text-font-size: var(--tv-font-size-sm, 12px);
    --tv-Locales-popper-li-padding-h: var(--tv-space-md, 8px);
  }
  
  // ... existing variables ...
}
examples/sites/demos/pc/app/drawer/use-through-method-composition-api.vue (1)

27-39: Consider enhancing notification consistency.

The implementation successfully replaces console.log with user-visible notifications. However, there's an inconsistency in the message content between open and close events.

Consider applying this change for better consistency:

    close: () =>
      Notify({
        type: 'info',
        title: 'close 事件',
+       message: '抽屉已关闭',
        position: 'top-right'
      })
examples/sites/demos/pc/app/area/custom-service.spec.ts (1)

Line range hint 1-41: Consider enhancing test coverage for locale-specific scenarios.

Given that this PR focuses on locale optimization, consider adding test cases for:

  1. Different locale settings and their impact on the area selection
  2. Edge cases in locale-specific formatting
  3. Locale switching behavior

Would you like me to help generate additional test cases for these scenarios?

examples/sites/demos/pc/app/drawer/use-through-method.vue (2)

21-23: Add null check for drawerInstance

Consider adding a null check to prevent potential runtime errors if the drawer instance hasn't been initialized.

 closeDrawer() {
+  if (!this.drawerInstance) return
   this.drawerInstance.close()
 }

33-45: Consider adding more context to close notification

While the open notification includes the drawer title, the close notification could be more informative.

 close: () =>
   Notify({
     type: 'info',
     title: 'close 事件',
+    message: '抽屉已关闭',
     position: 'top-right'
   })
packages/vue/src/locales/src/pc.vue (1)

16-21: Fix template indentation for better readability.

The template structure has improved with the addition of semantic classes, but the indentation is inconsistent. The closing tags and expressions should be properly aligned.

Apply this diff to fix the indentation:

-        <span class="tiny-locales__text"
-          >{{ state.text && t(state.text)
-          }}<span class="tiny-locales__icon">
-            <IconChevronDown v-show="!state.visible" />
-            <IconChevronUp v-show="state.visible" /> </span
-        ></span>
+        <span class="tiny-locales__text">
+          {{ state.text && t(state.text) }}
+          <span class="tiny-locales__icon">
+            <IconChevronDown v-show="!state.visible" />
+            <IconChevronUp v-show="state.visible" />
+          </span>
+        </span>
examples/sites/demos/pc/app/area/custom-service.vue (1)

33-33: Consider updating English names to match simplified Chinese names

The Chinese names have been simplified by removing "Marketing与", but the corresponding English names still contain "Marketing and". Consider updating the English names for consistency:

  • Consumer BG Marketing and Sales Service DeptConsumer BG Sales Service Dept
  • Consumer BG Marketing and Sales Service Dept (Dongguan)Consumer BG Sales Service Dept (Dongguan)

Also applies to: 46-46

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 299d7fe and dd6e2c5.

📒 Files selected for processing (15)
  • examples/sites/demos/pc/app/amount/size-composition-api.vue (1 hunks)
  • examples/sites/demos/pc/app/amount/size.vue (1 hunks)
  • examples/sites/demos/pc/app/area/custom-service-composition-api.vue (2 hunks)
  • examples/sites/demos/pc/app/area/custom-service.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/area/custom-service.vue (2 hunks)
  • examples/sites/demos/pc/app/drawer/use-through-method-composition-api.vue (2 hunks)
  • examples/sites/demos/pc/app/drawer/use-through-method.vue (3 hunks)
  • examples/sites/demos/pc/app/locales/custom-service-composition-api.vue (1 hunks)
  • examples/sites/demos/pc/app/locales/custom-service.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/locales/custom-service.vue (1 hunks)
  • packages/theme/src/area/index.less (1 hunks)
  • packages/theme/src/area/vars.less (1 hunks)
  • packages/theme/src/locales/index.less (2 hunks)
  • packages/theme/src/locales/vars.less (1 hunks)
  • packages/vue/src/locales/src/pc.vue (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/theme/src/area/vars.less
🔇 Additional comments (20)
examples/sites/demos/pc/app/amount/size-composition-api.vue (2)

1-8: LGTM! Well-structured size demonstration.

The component effectively demonstrates different size variants of TinyAmount with consistent data bindings.


1-8: Verify size prop values against component API.

Let's verify that all used size values ("medium", "small", "mini") are valid according to the TinyAmount component's API documentation.

examples/sites/demos/pc/app/amount/size.vue (1)

1-8: The size variations are well-structured and comprehensive.

The demo effectively showcases all available size variants of the TinyAmount component (medium, default, small, and mini) while maintaining consistent data binding across all instances.

examples/sites/demos/pc/app/locales/custom-service-composition-api.vue (1)

14-14: LGTM! Improved locale format standardization.

The change to use underscore-separated locale codes (e.g., 'zh_CN' instead of 'zhCN') aligns better with BCP 47 language tag standards and common i18n library practices.

Also applies to: 18-18

packages/theme/src/area/index.less (2)

14-14: LGTM: Good practice importing vars.less

The addition of vars.less import aligns with the best practice of centralizing CSS variables and maintaining consistent styling across components.


21-25: Verify the impact of width reduction

The changes look good from a maintainability perspective, replacing hardcoded values with CSS variables. However, the width reduction from 270px to 220px might affect the layout.

Let's verify if this width change impacts other components:

A few suggestions:

  1. The double semicolon on line 25 should be fixed
  2. Consider adding a CSS variable for the width value as well, for consistency with other properties

Apply this fix for the double semicolon:

-    margin-bottom: var(--tv-Area-select-margin-bottom);;
+    margin-bottom: var(--tv-Area-select-margin-bottom);

Consider extracting the width to a CSS variable:

-    width: 220px;
+    width: var(--tv-Area-select-width);
✅ Verification successful

Width reduction from 270px to 220px appears safe to implement

The width change from 270px to 220px in the Area component appears to be isolated and safe to implement. While other components (link-menu, grid, amount) use 270px width, they are independent components with no direct styling relationships or inheritance from the Area component. The only connections found are basic imports which don't share styles.

  • Fix the double semicolon on line 25 as suggested
  • Consider extracting the width to a CSS variable for better maintainability
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any related width definitions or dependencies
rg -t less "270px" packages/theme/src/

Length of output: 378


Script:

#!/bin/bash
# Let's check if these files have any references to the Area component or its classes
rg -A 5 -B 5 "Area" packages/theme/src/link-menu/index.less packages/theme/src/grid/filter.less packages/theme/src/grid/button.less packages/theme/src/amount/index.less

# Also check for any imports or mixins that might connect these components
rg "@import.*area|from.*area" packages/theme/src/

Length of output: 403

examples/sites/demos/pc/app/locales/custom-service.vue (1)

19-19: Verify breaking change impact: Locale format change.

The locale format has changed from camelCase ('zhCN') to underscore format ('zh_CN'). While this better aligns with ISO language codes, it's a breaking change that could impact consuming code.

packages/theme/src/locales/vars.less (1)

1-11: LGTM: Copyright header is properly formatted.

The copyright notice and license information are complete and well-structured.

examples/sites/demos/pc/app/drawer/use-through-method-composition-api.vue (1)

9-9: LGTM: Import statement is properly organized.

The addition of Notify import is well-placed alongside other @opentiny/vue imports and aligns with the enhancement of user feedback mechanisms.

examples/sites/demos/pc/app/area/custom-service.spec.ts (1)

Line range hint 37-41: LGTM! Test expectations correctly updated to match component changes.

The updated test expectations align with the changes made in the related components where "Marketing与" was removed from the company name.

Let's verify the consistency of this change across related files:

✅ Verification successful

Test expectations correctly aligned with component implementation

The verification confirms that the company name format "A公司销售服务部" is consistently used across all related files:

  • custom-service.vue
  • custom-service-composition-api.vue
  • custom-service.spec.ts

The test expectations match the actual implementation in both the regular and composition API versions of the component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the company name change is consistent across components and tests
# Expected: All occurrences should use "A公司销售服务部" instead of "A公司 Marketing与销售服务部"

rg -l "A公司.*销售服务部"

Length of output: 196


Script:

#!/bin/bash
# Let's check the actual content of these files to verify the company name format
rg -A 2 "A公司.*销售服务部" examples/sites/demos/pc/app/area/custom-service-composition-api.vue examples/sites/demos/pc/app/area/custom-service.spec.ts examples/sites/demos/pc/app/area/custom-service.vue

Length of output: 1828

examples/sites/demos/pc/app/drawer/use-through-method.vue (2)

8-8: LGTM! Good enhancement with Notify import

The addition of Notify improves user feedback compared to console logging.


Line range hint 24-63: Verify handling of multiple drawer instances

The current implementation allows opening multiple drawer instances. Consider adding a check to prevent this or close existing drawer before opening a new one.

packages/theme/src/locales/index.less (4)

14-14: LGTM: Import statement follows conventions

The addition of vars.less import is properly placed and follows the project's import conventions.


28-33: LGTM: Well-structured text component styles

The text component styles follow BEM naming conventions and make good use of CSS variables and flexbox for alignment.


60-62: LGTM: Proper hover state implementation

The hover state implementation correctly uses CSS variables, maintaining consistency with the theme system.


21-27: Verify the mixin implementation

The injection of variables through the mixin is a good practice for maintainability. However, we should verify the mixin's implementation.

✅ Verification successful

Let me check the actual implementation of the mixin to verify it's properly defined.


Mixin implementation is correct and well-structured

The .inject-Locales-vars() mixin is properly defined in vars.less with clear variable declarations using CSS custom properties. The variables are well-documented with Chinese comments and follow consistent naming conventions. The --tv-Locales-svg-margin-left variable used in the code is correctly defined in the mixin.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the mixin is properly defined in vars.less
rg -l "inject-Locales-vars" "packages/theme/src/locales/"

Length of output: 130


Script:

#!/bin/bash
# Check the content of the mixin definition
rg -A 5 "inject-Locales-vars" packages/theme/src/locales/vars.less

Length of output: 237

examples/sites/demos/pc/app/area/custom-service-composition-api.vue (3)

26-29: Consider aligning English name with simplified Chinese name

The Chinese name has been simplified by removing "Marketing与", but the English name still contains "Marketing and". This inconsistency might cause confusion for bilingual users or systems.

Consider updating the English name to match:

-      name_en: 'Consumer BG Marketing and Sales Service Dept',
+      name_en: 'Consumer BG Sales Service Dept',

Line range hint 26-42: LGTM - Naming simplification aligns with locales optimization

The changes consistently simplify the Chinese company names while maintaining the component's functionality. The mock data structure and promise-based API remain properly implemented.


39-42: Verify naming convention changes across the codebase

The naming simplification pattern is consistent with the previous entry. However, there's a similar English-Chinese name mismatch.

Let's verify if there are other instances of this naming pattern that need to be updated:

✅ Verification successful

The previous script failed because the file type 'vue' wasn't recognized. Let's try searching without file type restriction and also include a more specific search for the company name pattern to gather comprehensive information about naming conventions in the codebase.


The naming pattern is consistently applied across the codebase

The search results show that the simplified Chinese naming convention ('A公司销售服务部') is consistently used across all relevant files, including:

  • custom-service.spec.ts
  • custom-service.vue
  • custom-service-composition-api.vue

The English translations ('Consumer BG Marketing and Sales Service Dept') are also consistent throughout. While there is a difference between the Chinese and English versions (Marketing appears in English but not Chinese), this appears to be an intentional localization choice rather than an inconsistency that needs to be fixed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar naming patterns in the codebase
rg -i "marketing.*销售|销售.*marketing" --type vue

Length of output: 76


Script:

#!/bin/bash
# Search for similar naming patterns in all files, without file type restriction
rg -i "marketing.*销售|销售.*marketing"

# Also search for the specific company name pattern
rg -i "A公司.*销售|Consumer BG.*Marketing"

Length of output: 1066

examples/sites/demos/pc/app/area/custom-service.vue (1)

33-33: Verify consistent naming convention across demos

The company names have been simplified. Let's verify this naming convention is consistently applied across all demo files.

Also applies to: 46-46

✅ Verification successful

Let me search for both the specific department name and company references to understand the naming convention better. The previous search might have been too specific, so I'm broadening it to catch all variations.


Company naming convention is consistently applied

The naming convention is consistent across the demo files. All instances use simplified naming:

  • 'A公司销售服务部'
  • 'A公司销售服务部(东莞)'
  • '火星终端子公司'

These patterns appear identically in both custom-service.vue and custom-service-composition-api.vue, showing consistent naming across different implementations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar company name patterns in other demo files
rg -g '*.vue' "Marketing.*销售服务部" examples/sites/demos/

Length of output: 56


Script:

#!/bin/bash
# Search for company names with 销售服务部 pattern in demo files
rg -g '*.vue' "销售服务部" examples/sites/demos/

# Also search for company names with "公司" pattern to see the naming convention
rg -g '*.vue' "公司" examples/sites/demos/pc/app/area/

Length of output: 1053

// 弹框选项间距
--tv-Locales-popper-li-padding-h: var(--tv-space-xl, 16px);
// 弹框选项行高
--tv-Locales-popper-li-line-heigaht: 30px;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in CSS property name.

The property name contains a typo: "line-heigaht" should be "line-height".

-  --tv-Locales-popper-li-line-heigaht: 30px;
+  --tv-Locales-popper-li-line-height: 30px;
📝 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.

Suggested change
--tv-Locales-popper-li-line-heigaht: 30px;
--tv-Locales-popper-li-line-height: 30px;

},
// 通过属性 customSlots 设置插槽, 插槽内容可以是 string | VNode | ({h, $drawer}) => VNode
customSlots: {
// 使用 h 函数
default: (h) => h('p', { class: '' }, '抽屉主体内容。'),
// 返回 VNode 节点的方法, 可通过参数中 $drawer 访问到组件实例
headerRight: () => <IconHelp></IconHelp>,
headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider moving styles to CSS classes.

While inline styles work, moving them to CSS classes would improve maintainability and reusability.

Consider this approach:

-    headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
+    headerRight: () => <IconHelp class="drawer-header-icon"></IconHelp>,

And add to your component's style section:

<style>
.drawer-header-icon {
  width: 20px;
  height: 20px;
}
</style>

Comment on lines +52 to +57
headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
// 直接赋值 VNode
footer: (
<Button type="primary" onClick={this.closeDrawer}>
<TinyButton type="primary" onClick={this.closeDrawer}>
点击关闭
</Button>
</TinyButton>
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider moving inline styles to CSS classes

While the inline styles work, consider moving them to CSS classes for better maintainability and reusability.

-headerRight: () => <IconHelp style="width:20px;height:20px"></IconHelp>,
+headerRight: () => <IconHelp class="drawer-header-icon"></IconHelp>,

Add to your style section:

<style>
.drawer-header-icon {
  width: 20px;
  height: 20px;
}
</style>

Comment on lines +34 to +42
&__icon {
display: inline-flex;
align-items: center;
justify-content: center;
svg {
width: var(--tv-Locales-svg-size);
height: var(--tv-Locales-svg-size);;
fill: var(--tv-Locales-text-color);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix double semicolon in height property

There's a syntax error in the SVG height property.

-      height: var(--tv-Locales-svg-size);;
+      height: var(--tv-Locales-svg-size);
📝 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.

Suggested change
&__icon {
display: inline-flex;
align-items: center;
justify-content: center;
svg {
width: var(--tv-Locales-svg-size);
height: var(--tv-Locales-svg-size);;
fill: var(--tv-Locales-text-color);
}
&__icon {
display: inline-flex;
align-items: center;
justify-content: center;
svg {
width: var(--tv-Locales-svg-size);
height: var(--tv-Locales-svg-size);
fill: var(--tv-Locales-text-color);
}

Comment on lines +57 to +58
padding: 0 var(--tv-Locales-popper-li-padding-h);
line-height: var(--tv-Locales-popper-li-line-heigaht);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in line-height variable name

There's a typo in the line-height variable name that could cause styling issues.

-        line-height: var(--tv-Locales-popper-li-line-heigaht);
+        line-height: var(--tv-Locales-popper-li-line-height);
📝 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.

Suggested change
padding: 0 var(--tv-Locales-popper-li-padding-h);
line-height: var(--tv-Locales-popper-li-line-heigaht);
padding: 0 var(--tv-Locales-popper-li-padding-h);
line-height: var(--tv-Locales-popper-li-line-height);

Comment on lines +48 to 49
IconChevronUp: IconUpWard(),
IconChevronDown: IconChevronDown()
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider renaming the component registration for clarity.

The component is imported as IconUpWard but registered as IconChevronUp, which could be confusing for maintenance. Consider updating the registration name to match the imported component name.

Apply this diff to maintain consistency:

-    IconChevronUp: IconUpWard(),
+    IconUpward: IconUpWard(),

Then update the template usage:

-            <IconChevronUp v-show="state.visible" />
+            <IconUpward v-show="state.visible" />

Committable suggestion was skipped due to low confidence.

@@ -39,13 +39,13 @@
import { renderless, api } from '@opentiny/vue-renderless/locales/vue'
import { use, enUS, zhCN } from '@opentiny/vue-locale'
import TinyPopover from '@opentiny/vue-popover'
import { IconChevronUp, IconChevronDown } from '@opentiny/vue-icon'
import { IconUpWard, IconChevronDown } from '@opentiny/vue-icon'
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the search results, I can generate the final response since I have enough information to verify the icon change impact:

The icon change from IconChevronUp to IconUpWard appears to be inconsistent with the codebase patterns

The search results reveal that:

  • IconChevronUp and IconChevronDown are consistently used together as a pair across multiple components (user-link, user-contact, time-line, roles, etc.)
  • The IconUpWard icon is rarely used in the codebase, with only one other usage in the cascader component
  • The current change breaks this established pattern in the locales component by pairing IconChevronDown with IconUpWard

This change could lead to visual inconsistency since:

  • The component uses both icons for toggling states (visible/not visible)
  • IconChevronDown and IconChevronUp are designed as a matching pair
  • IconUpWard may have different styling/dimensions than the chevron icons
🔗 Analysis chain

Verify the visual impact of replacing IconChevronUp with IconUpWard.

The icon change from IconChevronUp to IconUpWard might affect the visual consistency of the component. Please ensure that:

  1. The new icon maintains visual harmony with IconChevronDown
  2. The icon size and style match the design system

Let's check if these icons are used consistently across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the usage patterns of IconChevronUp and IconUpWard
# to ensure consistent icon usage across the codebase

echo "Checking IconChevronUp usage:"
rg "IconChevronUp" -A 2

echo -e "\nChecking IconUpWard usage:"
rg "IconUpWard" -A 2

echo -e "\nChecking if these icons are paired with IconChevronDown:"
rg "IconChevronDown" -A 2

Length of output: 35861


export default defineComponent({
name: $prefix + 'Locales',
components: {
TinyPopover,
IconChevronUp: IconChevronUp(),
IconChevronUp: IconUpWard(),
Copy link
Member

Choose a reason for hiding this comment

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

这里修改了模板,需要确认下是否对saas主题造成影响

@Youyou-smiles Youyou-smiles deleted the lty/feat-locales branch November 7, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request (功能增强)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants