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(theme): [tree,tooltip,transfer, cascader] update unsolved smb components #2124

Merged
merged 9 commits into from
Sep 14, 2024

Conversation

shenjunjian
Copy link
Collaborator

@shenjunjian shenjunjian commented Sep 13, 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

  • New Features

    • Updated icons for dropdown functionality in the Cascader component.
    • Enhanced styling for input elements in the transfer panel.
  • Bug Fixes

    • Corrected the assignment of tree icons to ensure accurate visual representation.
  • Style

    • Improved padding and margin settings for dropdown menus and tooltips.
    • Adjusted background colors for tree nodes and tooltips to enhance visibility.
    • Streamlined tooltip styling for better maintainability.
  • Documentation

    • Updated variable names for better clarity in the tooltip and tree components.

Copy link

coderabbitai bot commented Sep 13, 2024

Walkthrough

The pull request introduces several modifications across various components, primarily focusing on icon assignments, styling adjustments, and variable updates. Key changes include swapping icon assignments in tree-related components, adjustments to padding and margins in the Cascader component's styling, and a refactor of tooltip and transfer panel styles. Additionally, the tooltip's visual properties have been enhanced, and the Cascader component has updated icon imports to align with new naming conventions.

Changes

File Path Change Summary
examples/sites/demos/pc/app/tree/icons-composition-api.vue, examples/sites/demos/pc/app/tree/icons.vue Swapped assignments of shrinkIcon and expandIcon variables, altering their respective icon functions.
packages/theme/src/cascader-menu/vars.less Updated vertical padding for dropdown menu list and removed outer margins.
packages/theme/src/cascader/index.less Removed border from dropdown element, simplifying its visual presentation.
packages/theme/src/cascader/vars.less Changed --ti-cascader-dropdown-box-shadow variable to a more specific shadow style.
packages/theme/src/mixins/tooltip.less Simplified tooltip styling by removing specific placement rules and generalizing the .popper__arrow class.
packages/theme/src/tooltip/index.less Refactored .popper__arrow styling to use a div for the arrow shape, removing border properties and adjusting positioning.
packages/theme/src/tooltip/vars.less Increased tooltip arrow width and height, adjusted border radius, and modified box shadow for enhanced visual appearance.
packages/theme/src/transfer/index.less Added new CSS rules for input elements in the transfer panel, including a new bottom border color and padding adjustments.
packages/theme/src/transfer/vars.less Minor formatting adjustments and increased padding for the panel filter.
packages/theme/src/tree/index.less Updated background color for "current" tree node content to enhance visual styling.
packages/theme/src/tree/vars.less Modified --ti-tree-node-highlight-bg-color variable to change the highlight color of tree nodes.
packages/vue/src/cascader/src/pc.vue Updated icon imports from iconChevronDown and iconChevronUp to IconTriangleDown and IconUpWard, respectively.

Possibly related PRs

Poem

🐇 In the garden where icons play,
A swap of colors brightens the day.
With shadows deep and borders gone,
The Cascader blooms, the tooltips shone.
A hop, a skip, through styles we glide,
In this code, our joy we hide! 🌼

Tip

OpenAI O1 model for chat
  • We have deployed OpenAI's latest O1 model for chat.
  • OpenAI claims that this model has superior reasoning capabilities than their GPT-4o model.
  • Please share any feedback with us in the discussions post.

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cb3c0b4 and 9ad105e.

Files selected for processing (3)
  • examples/sites/demos/pc/app/tree/icons.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/tree/props.spec.ts (1 hunks)
  • packages/theme/src/tree/vars.less (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • packages/theme/src/tree/vars.less
Additional comments not posted (4)
examples/sites/demos/pc/app/tree/props.spec.ts (1)

11-11: LGTM!

The change in the expected color aligns with the intention to update the styling of disabled elements. The test assertion syntax and logic are correct.

examples/sites/demos/pc/app/tree/icons.spec.ts (3)

12-12: Ensure the new locator is still specific enough.

The change to the icon2 locator simplifies the selection by targeting the SVG element directly instead of a specific path within it. This can improve the robustness of the test.

However, please ensure that selecting the SVG element as a whole still uniquely identifies the desired icon and doesn't introduce any ambiguity.


16-16: Confirm the class is specific enough.

The change to validate the presence of the tiny-tree-node__expand-icon class on icon1 is a more flexible and maintainable approach compared to checking detailed SVG attributes or styles.

However, please confirm that this class is specific enough to uniquely identify the desired icon state and doesn't introduce any false positives.


20-20: LGTM!

The change to validate the presence of the tiny-tree-node__expand-icon class on icon2 maintains consistency with the approach used for icon1.

As mentioned in the previous comment, please ensure this class is specific enough to uniquely identify the desired icon state for icon2 as well.


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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

[e2e-test-warn]
The component to be tested is missing.

The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug".

Please make sure you've read our contributing guide

@shenjunjian shenjunjian changed the title update unsolved smb components [tree,tooltip,transfer, cascader] update unsolved smb components Sep 13, 2024
@shenjunjian shenjunjian changed the title [tree,tooltip,transfer, cascader] update unsolved smb components fix(theme): [tree,tooltip,transfer, cascader] update unsolved smb components Sep 13, 2024
@github-actions github-actions bot added the bug Something isn't working label Sep 13, 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: 0

Outside diff range and nitpick comments (1)
packages/theme/src/tooltip/index.less (1)

Partial implementation of class name change detected

The change from .tooltip-light to .tooltip-type has been partially implemented across the codebase. While the mixin name has been updated in both theme-mobile and theme-saas packages, there's still an instance of .tooltip-light usage in packages/theme-saas/src/tooltip/index.less. Consider the following actions:

  • Update the .is-light class in packages/theme-saas/src/tooltip/index.less to use the new .tooltip-type mixin.
  • Double-check if any other files in the codebase might still be using the old .tooltip-light mixin.
  • Ensure consistency across all theme packages in the usage of the new .tooltip-type mixin.
Analysis chain

Line range hint 139-143: Verify the class name change in the codebase.

The renaming of the class for light tooltips from .tooltip-light to .tooltip-type looks good. However, ensure that all references to the old class name have been updated in the codebase.

Run the following script to verify the usage of the old class name:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to the old class name `.tooltip-light` have been updated.

# Test: Search for the old class name usage. Expect: No occurrences of the old class name.
rg --type less $'\.tooltip-light'

Length of output: 383


Script:

#!/bin/bash
# Description: Check the context of .tooltip-light usage and search for .tooltip-type in the affected files

# Check the context of .tooltip-light usage
echo "Checking .tooltip-light usage:"
rg --type less -C 5 '\.tooltip-light' packages/theme-saas/src/tooltip/index.less packages/theme-saas/src/mixins/tooltip.less packages/theme-mobile/src/mixins/tooltip.less

echo "\nSearching for .tooltip-type:"
rg --type less '\.tooltip-type' packages/theme-saas/src/tooltip/index.less packages/theme-saas/src/mixins/tooltip.less packages/theme-mobile/src/mixins/tooltip.less

Length of output: 3713

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 37f275f and cb3c0b4.

Files selected for processing (13)
  • examples/sites/demos/pc/app/tree/icons-composition-api.vue (1 hunks)
  • examples/sites/demos/pc/app/tree/icons.vue (1 hunks)
  • packages/theme/src/cascader-menu/vars.less (1 hunks)
  • packages/theme/src/cascader/index.less (1 hunks)
  • packages/theme/src/cascader/vars.less (1 hunks)
  • packages/theme/src/mixins/tooltip.less (1 hunks)
  • packages/theme/src/tooltip/index.less (2 hunks)
  • packages/theme/src/tooltip/vars.less (4 hunks)
  • packages/theme/src/transfer/index.less (1 hunks)
  • packages/theme/src/transfer/vars.less (2 hunks)
  • packages/theme/src/tree/index.less (1 hunks)
  • packages/theme/src/tree/vars.less (1 hunks)
  • packages/vue/src/cascader/src/pc.vue (2 hunks)
Files skipped from review due to trivial changes (1)
  • packages/theme/src/transfer/vars.less
Additional comments not posted (22)
packages/theme/src/mixins/tooltip.less (1)

17-18: Simplified tooltip arrow styling looks good, but verify visual consistency.

The change to introduce a generalized styling for the .popper__arrow class using the @bgcolor variable is a good simplification. It reduces code duplication and makes the styling more maintainable.

However, please verify that this change does not introduce any visual inconsistencies or regressions in the tooltip appearance across different placements (top, bottom, left, right). Ensure that the tooltips still look visually consistent and aligned with the design specifications after this modification.

packages/theme/src/cascader-menu/vars.less (1)

21-21: LGTM!

The changes to the vertical padding and removal of outer margins for the dropdown menu list are consistent with the provided list of alterations. The increased vertical spacing and simplified styling rules should improve the visual appearance and maintainability of the component.

examples/sites/demos/pc/app/tree/icons-composition-api.vue (1)

26-27: Verify if the icon swap is intentional.

The assignments of shrinkIcon and expandIcon have been swapped. shrinkIcon now uses iconExpand() and expandIcon uses iconPutAway().

Please confirm if this change is intentional and aligns with the desired behavior of the tree component. If so, the change looks good to me.

examples/sites/demos/pc/app/tree/icons.vue (1)

31-32: LGTM!

The change in the icon assignments for shrinkIcon and expandIcon is intentional and aligns with the intended functionality or visual representation of the icons.

packages/theme/src/tooltip/vars.less (6)

14-15: LGTM!

The increased tooltip arrow width from 8px to 12px will make the arrow more prominent, enhancing the visual appearance of the tooltip.


16-17: LGTM!

The new CSS custom property --ti-tooltip-popper-border-height with a value of 20px complements the arrow width property and provides control over the height of the tooltip arrow.


18-19: LGTM!

The adjustment of the tooltip arrow border radius from 8px to 2px will make the arrow less rounded, providing a sharper appearance.


48-48: LGTM!

The change of the info tooltip background color value from #1476FF to #1476ff is a stylistic change from uppercase to lowercase. It maintains the same color while following a consistent casing convention.


52-52: LGTM!

The change of the info tooltip border color value from #1476FF to #1476ff is a stylistic change from uppercase to lowercase, maintaining consistency with the background color change.


88-88: LGTM!

The modification of the tooltip box shadow from 0px 2px 12px 0px rgba(0,0,0,0.08) to 0px 5px 9px 0px rgba(51, 56, 84, 0.25) creates a more pronounced shadow effect, enhancing the visual prominence of the tooltip.

packages/theme/src/tooltip/index.less (3)

46-54: Refactor looks good!

The refactor of the .popper__arrow styling simplifies the code by replacing the CSS border-based triangular arrow with a div element. This improves the maintainability and readability of the code.


61-61: LGTM!

The adjustment to the arrow positioning for the top placement ensures proper alignment.


69-69: LGTM!

The adjustment to the arrow positioning for the bottom placement ensures proper alignment.

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

65-65: LGTM!

The change to --ti-cascader-dropdown-box-shadow updates the box shadow style for the Cascader dropdown to a more specific shadow (--ti-common-shadow-3-left), potentially enhancing or altering the depth and positioning of the shadow effect. This change is consistent with the AI-generated summary and appears to be a valid update.

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

72-72: Verify if using a text color for the background is intentional.

The change updates the background color of highlighted tree nodes from --ti-common-color-light to --ti-base-text-color-2. However, --ti-base-text-color-2 seems to be a text color variable based on its name.

Using a text color for the background might impact the contrast ratio and readability of the text inside the highlighted node. Please verify if this change is intentional and ensure that it meets the accessibility guidelines.

packages/theme/src/cascader/index.less (1)

107-107: LGTM!

The change to remove the border from the dropdown element is acceptable. It may impact the visual presentation slightly but does not introduce any issues.

packages/vue/src/cascader/src/pc.vue (2)

202-202: LGTM!

The icon import changes align with the list of alterations and are consistent with the icon usage in the template.


262-263: LGTM!

The component registration changes align with the list of alterations and are consistent with the import changes.

packages/theme/src/tree/index.less (1)

96-96: LGTM!

The change to use --ti-tree-node-highlight-bg-color for the background color of the current tree node's content, when not showing a checkbox, looks good. It should help provide a clearer visual distinction and improve the user experience.

packages/theme/src/transfer/index.less (3)

280-282: LGTM!

The changes enhance the styling of the input element within the transfer panel component by setting a new bottom border color and adjusting the padding. The use of !important flag ensures the border color takes precedence over other styles.


284-286: LGTM!

The changes define the positioning for the prefix class within the input element by setting it to the left. This enhances the layout of the input field.


288-289: LGTM!

The changes define the positioning for the suffix class within the input element by setting it to the right with a 4px offset. This enhances the layout of the input field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants