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

refactor(tag-group): [tag-group] update tag-group theme #2271

Merged
merged 4 commits into from
Oct 19, 2024

Conversation

shenjunjian
Copy link
Collaborator

@shenjunjian shenjunjian commented Oct 15, 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

重构 tag-group theme
补充一个示例

Summary by CodeRabbit

  • New Features
    • Introduced new Vue components for tag groups with interactive click events.
    • Added a demo for basic usage of the tag group component.
  • Bug Fixes
    • Corrected text filters in tests to align with updated tag data.
    • Updated expected CSS properties in tests for various tag sizes and themes.
  • Documentation
    • Enhanced documentation for demo entries, including descriptions in both Chinese and English.
  • Style
    • Improved CSS structure and layout for the tag group component, including dynamic class bindings and adjustments to expected styles in tests.

@github-actions github-actions bot added the refactoring Refactoring (重构) label Oct 15, 2024
Copy link

coderabbitai bot commented Oct 15, 2024

Walkthrough

This pull request introduces several new Vue components and modifies existing ones within the tag group feature. New components include more-composition-api.vue and more.vue, both implementing a tiny-tag-group component that handles click events on tags. Additionally, formatting adjustments were made in various files to enhance consistency. The documentation for demo entries in tag-group.js was updated, and CSS structure changes were implemented in index.less and vars.less. Overall, the changes focus on enhancing the functionality and organization of the tag group component.

Changes

File Path Change Summary
examples/sites/demos/pc/app/tag-group/more-composition-api.vue New component added: <tiny-tag-group>, new method itemClick(item, i, e), reactive data array with 20 tags.
examples/sites/demos/pc/app/tag-group/more.vue New component TinyTagGroup, method itemClick(item, i, e), and data property initialized with 20 tags.
examples/sites/demos/pc/app/tag-group/tag-group-effect-composition-api.vue Formatting changes in template and style sections.
examples/sites/demos/pc/app/tag-group/tag-group-effect.vue Formatting changes in template and style sections.
examples/sites/demos/pc/app/tag-group/tag-group-event-composition-api.vue Removed tags "标签三" to "标签二十二", retaining only "标签一" and "标签二".
examples/sites/demos/pc/app/tag-group/tag-group-event.vue Removed tags "标签三" to "标签二十二", retaining only "标签一" and "标签二".
examples/sites/demos/pc/app/tag-group/tag-group-event.spec.js Updated text filters for test cases to reflect changes in tag data.
examples/sites/demos/pc/app/tag-group/tag-group-size-composition-api.vue Whitespace removed from tag size descriptions, newline added at end of style section.
examples/sites/demos/pc/app/tag-group/tag-group-size.vue Whitespace removed from tag size descriptions, newline added at end of style section.
examples/sites/demos/pc/app/tag-group/webdoc/tag-group.js New demo entry basic-usage, updates to existing demo entries and descriptions.
packages/theme/src/tag-group/index.less New variable @tag-prefix-cls, method replaced, layout properties adjusted.
packages/theme/src/tag-group/vars.less Variable and method updates for CSS structure.
packages/vue/src/tag-group/src/pc.vue Added dynamic class binding, removed inline styles, introduced <tiny-popover> component.

Possibly related PRs

Suggested reviewers

  • zzcr

🐰 In the meadow, tags do bloom,
With colors bright, they chase the gloom.
Click and see, a message clear,
A tiny group brings us cheer!
So hop along, let's celebrate,
New components make our tags so great! 🌼


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.

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: 5

🧹 Outside diff range and nitpick comments (9)
examples/sites/demos/pc/app/tag-group/tag-group-event.spec.js (1)

7-7: Consider renaming the tag3 variable for clarity.

The variable tag3 is now selecting the second tag ('标签二') instead of the third. To improve code readability and maintain consistency, consider renaming this variable to tag2.

Suggested change:

- const tag3 = page.locator('.tiny-tag ').filter({ hasText: '标签二' })
+ const tag2 = page.locator('.tiny-tag ').filter({ hasText: '标签二' })

Don't forget to update any subsequent uses of this variable in the test.

packages/theme/src/tag-group/vars.less (2)

15-25: Approve new CSS variables with a minor suggestion.

The new CSS variables provide excellent granular control over the tag group layout and use existing spacing variables for consistency. Well done!

A minor suggestion to improve clarity:

Consider updating the comment for --tv-TagGroup-item-margin-bottom to differentiate it from --tv-TagGroup-item-margin-right. For example:

- // 标签组的子项的间距
+ // 标签组的子项的底部间距

This change would make the distinction between horizontal and vertical spacing clearer.


13-25: Approve overall structure and naming conventions.

The overall structure and naming conventions in this file are excellent. The consistent use of the --tv-TagGroup-* prefix for CSS variables and the logical grouping of variables enhance readability and maintainability.

Consider adding English translations alongside the Chinese comments to improve accessibility for non-Chinese speaking developers. For example:

// 标签组的底部外间距 (Bottom margin of the tag group)
--tv-TagGroup-margin-bottom: var(--tv-space-lg);

This change would make the code more accessible to a wider range of developers while still maintaining the original Chinese comments.

packages/theme/src/tag-group/index.less (2)

20-24: LGTM: Good use of CSS variables and mixins

The replacement of .component-css-vars-tag-group() with .inject-TagGroup-vars() and the introduction of the --tv-TagGroup-margin-bottom CSS variable are good improvements. They enhance the component's flexibility and make it easier to maintain consistent spacing.

Consider adding a comment explaining the purpose of .inject-TagGroup-vars() for better code documentation.


49-50: Consider translating the comment to English

The comment provides useful context for the new .@{tag-group-prefix-cls}-popper class. However, for consistency and to improve maintainability for a potentially diverse team, consider translating the comment to English.

Suggested translation:

// Maintain margin for tag items within the popover
examples/sites/demos/pc/app/tag-group/more.vue (1)

5-7: Consider moving the Modal import.

The Modal import is only used in the itemClick method. For better code organization and to follow the principle of proximity, consider moving this import closer to where it's used or into the method itself if it's not used elsewhere in the file.

 <script>
-import { TagGroup, Modal } from '@opentiny/vue'
+import { TagGroup } from '@opentiny/vue'

 export default {
   // ... (rest of the component code)
   methods: {
     itemClick(item, i, e) {
+      const { Modal } = require('@opentiny/vue')
       Modal.message(`当前点击的是第${i + 1}个标签`)
     }
   }
 }
packages/vue/src/tag-group/src/pc.vue (2)

12-18: LGTM: Addition of popover for improved content handling

The introduction of the <tiny-popover> component is a good addition for managing overflow or additional content. The conditional rendering and prop settings are appropriate.

Consider adding a comment explaining the purpose of this popover for better code documentation.

 <tiny-popover
   v-if="state.showMore"
   placement="top-start"
   trigger="hover"
   width="auto"
   popper-class="tiny-tag-group-popper"
+ >
+   <!-- Popover for displaying additional tags when they overflow the main container -->
 >

19-28: LGTM: Consistent implementation of hidden tags

The implementation of hidden tags within the popover is well done. It maintains consistency with the main tags and efficiently uses a loop for rendering.

Consider extracting the common tag properties into a computed property or a separate component to reduce duplication between the main tags and hidden tags.

Example of extracting common properties:

<script setup>
const commonTagProps = computed(() => ({
  size: props.size,
  effect: props.effect
}))
</script>

<template>
  <tiny-tag
    v-for="(tag, i) in state.hiddenTags"
    :key="tag.name + i"
    v-bind="commonTagProps"
    :type="tag.type"
    @click="handelItemClick(tag, data.length - state.hiddenTags.length + i, $event)"
  >
    {{ tag.name }}
  </tiny-tag>
</template>
examples/sites/demos/pc/app/tag-group/webdoc/tag-group.js (1)

53-63: Good update to the 'more' demo entry

The renaming of this demo entry from 'tag-group-effect' to 'more' and the updated descriptions clearly explain the purpose of this demo, which is to showcase how the component handles overflow. The change in the code file reference to 'more.vue' aligns well with the new demo name.

However, there's a minor typo in the English description that could be improved:

Consider updating the English description as follows:

- 'The label group automatically identifies the length of the subitem. When the subitem is displayed in more than one row, more ICONS are displayed automatically. The mouse hover displays the remaining subitems'
+ 'The tag group automatically identifies the length of the subitems. When the subitems exceed one row, a "more" icon is automatically displayed at the end. Hovering over this icon shows the remaining subitems.'

This change improves clarity and corrects some grammatical issues.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e2a5fba and 92aac0b.

📒 Files selected for processing (13)
  • examples/sites/demos/pc/app/tag-group/more-composition-api.vue (1 hunks)
  • examples/sites/demos/pc/app/tag-group/more.vue (1 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect-composition-api.vue (2 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect.vue (2 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-event-composition-api.vue (0 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-event.spec.js (1 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-event.vue (0 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-size-composition-api.vue (2 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-size.vue (2 hunks)
  • examples/sites/demos/pc/app/tag-group/webdoc/tag-group.js (2 hunks)
  • packages/theme/src/tag-group/index.less (1 hunks)
  • packages/theme/src/tag-group/vars.less (1 hunks)
  • packages/vue/src/tag-group/src/pc.vue (1 hunks)
💤 Files with no reviewable changes (2)
  • examples/sites/demos/pc/app/tag-group/tag-group-event-composition-api.vue
  • examples/sites/demos/pc/app/tag-group/tag-group-event.vue
✅ Files skipped from review due to trivial changes (4)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect-composition-api.vue
  • examples/sites/demos/pc/app/tag-group/tag-group-effect.vue
  • examples/sites/demos/pc/app/tag-group/tag-group-size-composition-api.vue
  • examples/sites/demos/pc/app/tag-group/tag-group-size.vue
🧰 Additional context used
🔇 Additional comments (17)
examples/sites/demos/pc/app/tag-group/tag-group-event.spec.js (1)

7-10: Verify the intentionality of changes in tag selection and expected output.

The test has been modified to click on the second tag ('标签二') instead of the third, and the expected modal text has been updated accordingly. Please confirm if these changes are intentional and align with the expected behavior of the component after the refactoring.

To ensure these changes are consistent across the codebase, you may want to run the following check:

This will help identify if there are any other files that might need to be updated to reflect this change in behavior.

packages/theme/src/tag-group/vars.less (2)

13-25: Request information on removed variables.

The AI summary mentions that previous variables related to padding and icon color have been removed. Could you provide more information about these removed variables and confirm that their removal won't negatively impact existing styles or components?

To help assess the impact, please run the following script:

#!/bin/bash
# Description: Check for usage of removed variables in other files

# Test: Search for any references to padding or icon color variables in other less files
rg --type less "tv-(tag-group|TagGroup).*(padding|icon-color)"

# Note: If this returns any results, those files may need to be updated or the variables may need to be reintroduced.

13-13: Approve function name change, but verify impact.

The new function name .inject-TagGroup-vars() is more specific and follows a better naming convention. However, ensure that this change doesn't break any existing code that might be calling the old function name.

To verify the impact of this name change, run the following script:

examples/sites/demos/pc/app/tag-group/more-composition-api.vue (1)

1-3: LGTM: Template structure is clean and follows Vue conventions.

The template correctly uses the tiny-tag-group component, binding the data prop and listening to the item-click event. The naming conventions for both the component and the event follow Vue's recommendations.

packages/theme/src/tag-group/index.less (4)

16-17: LGTM: Good use of variables for class prefixes

The introduction of @tag-prefix-cls and @tag-group-prefix-cls variables is a good practice. It enhances maintainability by centralizing the definition of class prefixes, making it easier to update them across the stylesheet if needed.


26-33: LGTM: Improved margin handling for nested tags

The use of CSS variables (--tv-TagGroup-item-margin-right and --tv-TagGroup-item-margin-bottom) for tag margins is consistent with the earlier changes and provides good flexibility for theming. Removing the right margin from the last child is a good practice to prevent unnecessary spacing.


51-59: LGTM: Consistent styling for tags within popover

The use of .inject-TagGroup-vars() and the consistent application of margin styles for tags within the popover is excellent. This ensures visual consistency between the main tag group and its popover representation. The removal of the right margin from the last child is also consistent with the earlier implementation.


35-47: LGTM: Good addition for handling overflow items

The new class @{tag-group-prefix-cls}-has-more is a good addition for handling overflow items. The use of absolute positioning for the icon is appropriate and ensures it doesn't disrupt the layout.

Could you please clarify the reason for setting the SVG height to 22px? A comment explaining this specific value would be helpful for future maintenance.

examples/sites/demos/pc/app/tag-group/more.vue (2)

1-3: LGTM: Template structure is clean and follows Vue.js best practices.

The template correctly uses the TinyTagGroup component with proper data binding and event handling.


8-11: LGTM: Component definition is correct.

The component is properly defined and the TinyTagGroup is correctly registered as a local component.

packages/vue/src/tag-group/src/pc.vue (4)

2-2: LGTM: Dynamic class binding for improved flexibility

The addition of the dynamic class binding :class="{ 'tiny-tag-group-has-more': state.showMore }" is a good practice. It allows for conditional styling based on the component's state, which can enhance the visual feedback and flexibility of the component.


10-11: LGTM: Improved template formatting

The adjustment to the <tiny-tag> component's structure, moving the closing tag to a new line and properly using mustache brackets for content interpolation, enhances readability and adheres to Vue.js best practices.


Line range hint 29-33: LGTM: Well-implemented popover reference

The reference template for the popover is correctly implemented. Using a tag with an ellipsis icon is an intuitive way to indicate more content. The ref "more" is a good addition for potential programmatic access to this element.


Line range hint 1-62: Overall assessment: Solid improvements to the tag-group component

The changes in this file significantly enhance the tag-group component's functionality and readability. Key improvements include:

  1. Addition of dynamic class binding for more flexible styling.
  2. Implementation of a popover for handling overflow tags.
  3. Consistent handling of both visible and hidden tags.
  4. Improved template formatting for better readability.

These changes adhere to Vue.js best practices and should improve the component's usability. Consider the minor suggestions for further refinement, but overall, this is a well-implemented update.

examples/sites/demos/pc/app/tag-group/webdoc/tag-group.js (3)

11-22: Excellent addition of the 'basic-usage' demo!

The new demo entry provides a clear and comprehensive explanation of the fundamental usage of the tag-group component. The descriptions in both Chinese and English are well-written and cover the key properties (data, name, and type) effectively. This addition will greatly benefit users in understanding how to use the component.


25-36: Improved clarity for the 'tag-group-effect' demo

The updates to this demo entry enhance its specificity and relevance. The description now clearly explains the effect attribute and its possible values (dark / light / plain). The change in the code file reference from 'basic-usage.vue' to 'tag-group-effect.vue' better aligns with the demo's focus. These improvements will help users better understand and implement different themes for the tag group.


68-69: Name change for 'tag-group-event' demo is good, but consider updating the description

The change from 'Click 事件' to '事件' (and 'click event' to 'event' in English) makes the demo name more general, which is good if the component supports multiple types of events.

However, the description still specifically mentions only the 'item-click' event. To maintain consistency, please review the description and consider either:

  1. Updating it to cover all supported events if there are more than just 'item-click'.
  2. If 'item-click' is the only event, consider reverting the name change to keep it specific.

To help verify the available events, you can run the following script:

This will help identify all events emitted by the tag-group component.

Comment on lines +5 to +8
<script setup>
import { ref } from 'vue'
import { TagGroup as TinyTagGroup, Modal } from '@opentiny/vue'

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 optimizing imports for better tree-shaking.

The current imports are correct, but for better tree-shaking, consider importing only the specific components you need from '@opentiny/vue'.

You could refactor the imports like this:

import { ref } from 'vue'
import { TagGroup as TinyTagGroup, Modal } from '@opentiny/vue'

This approach can potentially reduce your bundle size by allowing the bundler to exclude unused components from '@opentiny/vue'.

Comment on lines +9 to +76
const data = ref([
{
name: '标签一'
},
{
name: '标签二'
},
{
name: '标签三'
},
{
name: '标签四'
},
{
name: '标签五'
},
{
name: '标签六'
},
{
name: '标签七'
},
{
name: '标签八'
},
{
name: '标签九'
},
{
name: '标签十'
},
{
name: '标签十一'
},
{
name: '标签十二'
},
{
name: '标签十三'
},
{
name: '标签十四'
},
{
name: '标签十五'
},
{
name: '标签十六'
},
{
name: '标签十七'
},
{
name: '标签十八'
},
{
name: '标签十九'
},
{
name: '标签二十'
},
{
name: '标签二十一'
},
{
name: '标签二十二'
}
])
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 generating the data array programmatically.

While the current implementation works, creating the data array manually can be error-prone and harder to maintain, especially if you need to add or remove tags in the future.

You could refactor the data creation like this:

const data = ref(
  Array.from({ length: 22 }, (_, i) => ({
    name: `标签${(i + 1).toString().padStart(2, '0')}`
  }))
)

This approach is more concise, easier to maintain, and less prone to errors when modifying the number of tags.

Comment on lines +78 to +80
function itemClick(item, i, e) {
Modal.message(`当前点击的是第${i + 1}个标签`)
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize the itemClick function and consider adding error handling.

The current implementation works, but there's room for improvement in terms of parameter usage and error handling.

Consider the following improvements:

  1. Remove unused parameters if they're not needed.
  2. Use the item parameter to display the tag name in the message.
  3. Add basic error handling.

Here's a suggested refactor:

function itemClick(item, index) {
  try {
    Modal.message(`You clicked on tag "${item.name}" (index: ${index + 1})`)
  } catch (error) {
    console.error('Error displaying click message:', error)
  }
}

This version provides more informative feedback and includes basic error handling.

Comment on lines +12 to +83
data() {
return {
data: [
{
name: '标签一'
},
{
name: '标签二'
},
{
name: '标签三'
},
{
name: '标签四'
},
{
name: '标签五'
},
{
name: '标签六'
},
{
name: '标签七'
},
{
name: '标签八'
},
{
name: '标签九'
},
{
name: '标签十'
},
{
name: '标签十一'
},
{
name: '标签十二'
},
{
name: '标签十三'
},
{
name: '标签十四'
},
{
name: '标签十五'
},
{
name: '标签十六'
},
{
name: '标签十七'
},
{
name: '标签十八'
},
{
name: '标签十九'
},
{
name: '标签二十'
},
{
name: '标签二十一'
},
{
name: '标签二十二'
}
]
}
},
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 generating the data array programmatically.

While the current data structure is correct, it's quite repetitive. You could simplify this by generating the array programmatically, which would make it easier to maintain and modify in the future.

Here's a suggested refactor:

data() {
  return {
    data: Array.from({ length: 22 }, (_, i) => ({
      name: `标签${this.numberToChinese(i + 1)}`
    }))
  }
},
methods: {
  numberToChinese(num) {
    const chineseNumbers = ['', '一', '二', '三', '四', '五', '六', '七', '八', '九', '十'];
    if (num <= 10) return chineseNumbers[num];
    if (num < 20) return `十${chineseNumbers[num - 10]}`;
    return `二十${chineseNumbers[num - 20]}`;
  }
}

This approach generates the same data array but is more concise and easier to modify if you need to change the number of tags or their naming convention.

Comment on lines +84 to +88
methods: {
itemClick(item, i, e) {
Modal.message(`当前点击的是第${i + 1}个标签`)
}
}
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 alternative ways to display clicked tag information.

While the current implementation works, showing a modal for every tag click might interrupt the user experience, especially for frequent interactions. Consider alternative ways to display this information, such as:

  1. Updating a text field in the component itself.
  2. Using a less intrusive notification system (e.g., a toast notification).
  3. Logging to console in development mode.

Here's an example of updating a text field in the component:

 <template>
-  <tiny-tag-group :data="data" @item-click="itemClick"></tiny-tag-group>
+  <div>
+    <tiny-tag-group :data="data" @item-click="itemClick"></tiny-tag-group>
+    <p v-if="lastClickedTag">Last clicked tag: {{ lastClickedTag }}</p>
+  </div>
 </template>

 <script>
-import { TagGroup, Modal } from '@opentiny/vue'
+import { TagGroup } from '@opentiny/vue'

 export default {
   // ... other code
   data() {
     return {
       data: [ /* ... */ ],
+      lastClickedTag: null
     }
   },
   methods: {
     itemClick(item, i, e) {
-      Modal.message(`当前点击的是第${i + 1}个标签`)
+      this.lastClickedTag = `第${i + 1}个标签 (${item.name})`
     }
   }
 }
 </script>

This approach provides immediate feedback without interrupting the user's workflow.

Committable suggestion was skipped due to low confidence.

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: 0

🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/tag-group/tag-group-size.spec.js (1)

8-17: Summary of TagGroup size adjustments

The changes in this file reflect updates to the expected dimensions and text positioning for medium, small, and mini-sized tags in the TagGroup component. These adjustments may be part of a broader design update.

Key points to consider:

  1. Ensure these changes are consistent with the latest design specifications.
  2. Verify that the new dimensions and line-heights don't cause any text overflow or alignment issues across different use cases.
  3. Confirm that these updates are applied consistently across the component library.
  4. Consider updating the component's documentation to reflect these new dimensions, if applicable.

To maintain consistency and ease future updates, consider centralizing these dimension values in a shared configuration file or CSS variables. This would allow for easier management of component sizes across the library.

examples/sites/demos/pc/app/tag-group/tag-group-effect.spec.js (2)

9-28: Dark theme color updates look good.

The changes to the dark theme colors are consistent and appear to follow a new design pattern. The use of white text (rgb(255, 255, 255)) on all dark backgrounds ensures good readability.

Consider adding a comment explaining the color scheme for better maintainability, e.g.:

// Dark theme color scheme:
// - First tag: Dark gray background
// - Subsequent tags: Distinct colors for each tag
// - All tags: White text for contrast

51-70: Plain theme color updates maintain consistency across themes.

The changes to the plain theme colors are well-aligned with the updates in other themes. The text colors now match the background colors of the corresponding dark theme tags, creating a consistent color scheme across all themes.

For improved consistency in the test structure, consider using a loop to test the plain theme tags, similar to how the dark and light themes are tested. This would make the test more maintainable and easier to update if new tags are added. For example:

const colors = ['rgb(25, 25, 25)', 'rgb(20, 118, 255)', 'rgb(92, 179, 0)', 'rgb(255, 136, 0)', 'rgb(242, 48, 48)'];
for (let i = 1; i <= 5; i++) {
  const plainTag = tagGroupsLocator.locator('.tiny-tag.tiny-tag--plain').filter({ hasText: `标签${i}` });
  await expect(plainTag).toHaveCSS('background-color', 'rgb(255, 255, 255)');
  await expect(plainTag).toHaveCSS('color', colors[i - 1]);
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 92aac0b and bd1da5a.

📒 Files selected for processing (5)
  • examples/sites/demos/pc/app/tag-group/basic-usage.spec.js (1 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect-composition-api.vue (2 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect.spec.js (1 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect.vue (3 hunks)
  • examples/sites/demos/pc/app/tag-group/tag-group-size.spec.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect-composition-api.vue
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/sites/demos/pc/app/tag-group/tag-group-effect.vue
🧰 Additional context used
🔇 Additional comments (11)
examples/sites/demos/pc/app/tag-group/tag-group-size.spec.js (3)

16-17: Verify subtle adjustments to mini-sized tags.

The expected CSS properties for mini-sized tags have been updated:

  • Height increased from 16px to 17px
  • Line-height decreased from 16px to 15px

These subtle changes may affect the appearance and text positioning of mini tags. Please ensure that:

  1. The 1px increase in height improves the overall appearance of the tag.
  2. The reduced line-height doesn't cause any text clipping or overflow.
  3. These adjustments align with the design team's intentions for mini tags.

To confirm these changes and check for potential issues, run the following script:

#!/bin/bash
# Search for mini tag implementations and their styling
rg --type=vue --type=js --type=less 'tiny-tag--mini' -C 3

13-13: Confirm line-height adjustment for small-sized tags.

The expected line-height for small-sized tags has been reduced from 20px to 18px, while the height remains at 20px. This change may affect text positioning within the tag.

Please verify that:

  1. This adjustment improves text alignment within the tag.
  2. The change is consistent with the design team's intentions.
  3. The new line-height doesn't cause any text overflow or clipping issues.

To check for potential text overflow issues, run the following script:

✅ Verification successful

To further investigate the usage of .tiny-tag--small, please run the following script:


Line-height adjustment for small-sized tags is confirmed.

The tests specify a line-height of 18px for small-sized tags while maintaining a height of 20px, ensuring proper text positioning within the tag.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for small tag implementations and their content
rg --type=vue --type=js 'tiny-tag--small' -C 5

Length of output: 85


Script:

#!/bin/bash
# Search for 'tiny-tag--small' in .vue, .js, and .less files
rg 'tiny-tag--small' --glob '*.vue' --glob '*.js' --glob '*.less' -C 5

Length of output: 1350


8-9: Verify design changes for medium-sized tags.

The expected CSS properties for medium-sized tags have been updated:

  • Height increased from 24px to 32px
  • Line-height decreased from 24px to 21px

These changes will affect the visual appearance of medium tags. Please ensure that:

  1. These changes align with the updated design specifications.
  2. The new dimensions are consistent with other components in the library.
  3. The changes have been approved by the design team.

To confirm these changes across the codebase, run the following script:

examples/sites/demos/pc/app/tag-group/basic-usage.spec.js (6)

10-11: LGTM: Updated tag1 styling consistent with theme refactor

The changes to tag1's background and text colors are in line with the PR's objective of refactoring the tag-group theme. The new colors provide a slightly higher contrast ratio, which may improve readability.


14-15: LGTM: Significant style update for tag2

The changes to tag2's styling represent a notable shift from a light gray to a light blue theme. This update is in line with the tag-group theme refactoring objective. The new color scheme might indicate that this tag now represents a primary or info state in the updated design system.


18-20: LGTM: Updated tag3 styling and improved code structure

The changes to tag3's color scheme, shifting from mint green to a more yellowish-green, align with the tag-group theme refactoring. The added empty line (20) improves the test file's readability by clearly separating the expectations for different tags.


22-23: LGTM: Subtle improvements to tag4 styling

The updates to tag4's colors, while subtle, are in line with the tag-group theme refactoring. The slightly darker background and brighter orange text may provide better contrast and improved readability. These changes contribute to the overall consistency of the updated theme.


26-27: LGTM: Updated tag5 styling with potential semantic meaning

The changes to tag5's colors, featuring a slightly darker pink background and brighter red text, align with the tag-group theme refactoring. This new color scheme might indicate that tag5 represents a warning or error state in the updated design system. The changes contribute to the overall consistency and clarity of the tag-group component.


Line range hint 1-29: LGTM: Comprehensive update of TagGroup styling expectations

The changes in this test file accurately reflect the refactoring of the tag-group theme, which was the main objective of this PR. All five tags have updated color expectations that are consistent with a cohesive design system update. The test file modifications ensure that the test suite remains in sync with the actual component styling changes, maintaining the integrity of the automated tests.

These updates contribute to:

  1. A more consistent color scheme across different tag states.
  2. Potentially improved readability and contrast for some tags.
  3. Clearer semantic meaning for different tag types (e.g., primary, warning, error).

Great job on keeping the test file up-to-date with the component changes!

examples/sites/demos/pc/app/tag-group/tag-group-effect.spec.js (2)

Line range hint 1-71: Overall, the TagGroup theme updates are well-implemented and consistent.

The changes in this test file reflect a comprehensive update to the TagGroup component's visual design across dark, light, and plain themes. The new color scheme provides better consistency and appears to improve the overall aesthetics of the component.

Key points:

  1. Dark theme now has more distinct colors for each tag.
  2. Light theme uses softer background colors with contrasting text.
  3. Plain theme maintains consistency by using text colors that match the dark theme backgrounds.

These updates should enhance the user experience and maintain visual harmony across different usage scenarios.

To ensure full compatibility with these changes, please verify that:

  1. The component's actual CSS implementation matches these new test expectations.
  2. Any documentation or design guidelines have been updated to reflect the new color scheme.
  3. Accessibility standards are still met with the new color combinations, especially for color contrast ratios.
#!/bin/bash
# Description: Verify documentation updates and check for potential accessibility issues

# Test: Search for updated color values in documentation
rg --type md "rgb\((25, 25, 25|20, 118, 255|92, 179, 0|255, 136, 0|242, 48, 48)\)" docs

# Note: For a proper accessibility check, you would typically use a specialized tool or service.
# This is a simplified check that looks for potential contrast ratio comments in the code.
rg --type css "contrast ratio" src/tag-group

30-49: Light theme color updates are consistent with the new design.

The changes to the light theme colors provide good contrast and follow a consistent pattern for each tag. The color combinations align well with the updates made in the dark theme section.

To ensure these color changes are reflected in the actual component styles, please run the following script:

This will help confirm that the test expectations match the actual component implementation.

@zzcr zzcr merged commit 50489a1 into opentiny:dev Oct 19, 2024
3 checks passed
@shenjunjian shenjunjian deleted the refactor-update-tag-group branch October 25, 2024 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring (重构)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants