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(dialog-box): [dialog-box] modify smb theme #2147

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

James-9696
Copy link
Collaborator

@James-9696 James-9696 commented Sep 19, 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

    • Reordered buttons in dialog box footers to emphasize the "Confirm" action.
    • Enhanced visual design with updated color schemes and spacing for dialog components.
  • Bug Fixes

    • Improved clarity and visual hierarchy within dialog box components.
  • Style

    • Adjusted styling for dialog box body and header to enhance visual distinction.

@github-actions github-actions bot added the enhancement New feature or request label Sep 19, 2024
Copy link

coderabbitai bot commented Sep 19, 2024

Walkthrough

The changes in this pull request primarily involve modifications to the dialog box component in a Vue.js application. Key updates include the reordering of button elements in the footer to emphasize the primary action, adjustments to styling in the associated LESS files, and the introduction of new color and padding variables. These alterations aim to enhance the user interface and improve the visual hierarchy of dialog box components.

Changes

Files Change Summary
examples/sites/demos/pc/app/dialog-box/right-dialog-composition-api.vue
examples/sites/demos/pc/app/dialog-box/right-dialog.vue
Reordered button elements in the footer, changing the "确定" (Confirm) button to primary and moving it before the "取消" (Cancel) button to enhance user interaction clarity.
packages/theme/src/dialog-box/index.less Modified styles for the dialog box components, including new color properties for the body and header, and added a border to the body section for visual distinction.
packages/theme/src/dialog-box/vars.less Updated color and padding variables, changing the body text color and introducing new variables for drawer content and padding adjustments to improve layout and consistency across the dialog components.

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • zzcr

🐇 In the dialog box, a change so bright,
The "Confirm" button now takes the light.
With colors and padding, all set in place,
A clearer UI, a friendlier space!
Hops of joy for the users, we cheer,
Thanks to these changes, the path is clear! 🐇


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

Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/dialog-box/right-dialog-composition-api.vue (1)

14-14: Consider adding a loading state to the primary button.

To provide visual feedback and prevent duplicate form submissions, consider adding a loading state to the primary "确定" (Confirm) button while the form is being processed.

Here's an example of how to add a loading state:

<tiny-button 
  type="primary" 
  :loading="isLoading"
  @click="handleConfirm"
>
  确定
</tiny-button>

In the component's script section, define the isLoading ref and handleConfirm method:

<script setup>
import { ref } from 'vue'

const isLoading = ref(false)

const handleConfirm = async () => {
  isLoading.value = true
  // Perform async confirmation logic...
  await asyncConfirmation()
  isLoading.value = false
  boxVisibility.value = false
}
</script>
examples/sites/demos/pc/app/dialog-box/right-dialog.vue (1)

Line range hint 1-54: Consider adding accessibility attributes.

To improve accessibility, consider adding the following attributes:

  • Add the aria-label attribute to the <tiny-button> elements to provide a clear and concise description of their purpose for screen readers.
  • Add the aria-modal="true" attribute to the <tiny-dialog-box> element to indicate that it is a modal dialog.

Apply this diff to add the accessibility attributes:

<template>
  <div>
-   <tiny-button @click="boxVisibility = true"> 右侧弹窗 </tiny-button>
+   <tiny-button @click="boxVisibility = true" aria-label="Open right side dialog"> 右侧弹窗 </tiny-button>
-   <tiny-dialog-box right-slide v-model:visible="boxVisibility" title="消息" modal-append-to-body>
+   <tiny-dialog-box right-slide v-model:visible="boxVisibility" title="消息" modal-append-to-body aria-modal="true">
      <template #title>
        <div class="header">
          <span class="title">标题区</span>
          <icon-help-circle class="icon"></icon-help-circle>
        </div>
      </template>
      <span>半屏弹窗内容</span>
      <template #footer>
-       <tiny-button plain @click="boxVisibility = false"> 取消 </tiny-button>
+       <tiny-button plain @click="boxVisibility = false" aria-label="Cancel"> 取消 </tiny-button>
-       <tiny-button type="primary" @click="boxVisibility = false"> 确定 </tiny-button>
+       <tiny-button type="primary" @click="boxVisibility = false" aria-label="Confirm"> 确定 </tiny-button>
      </template>
    </tiny-dialog-box>
  </div>
</template>
packages/theme/src/dialog-box/index.less (1)

Line range hint 1-298: Update documentation if necessary.

The PR checklist mentions that there are no documentation updates related to these changes. However, it's worth reviewing the documentation to determine if any updates are necessary.

Please consider:

  • Checking if the changes to the dialog box styling warrant any updates to the component's documentation, such as the theme customization guide or API reference.
  • Updating code examples or screenshots in the documentation to reflect the new visual appearance, if applicable.

Keeping the documentation in sync with the code changes improves the developer experience and helps users understand how to customize the dialog box theme effectively.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f0a1244 and 2d0fd05.

Files selected for processing (4)
  • examples/sites/demos/pc/app/dialog-box/right-dialog-composition-api.vue (1 hunks)
  • examples/sites/demos/pc/app/dialog-box/right-dialog.vue (1 hunks)
  • packages/theme/src/dialog-box/index.less (2 hunks)
  • packages/theme/src/dialog-box/vars.less (3 hunks)
Additional comments not posted (11)
examples/sites/demos/pc/app/dialog-box/right-dialog-composition-api.vue (2)

Line range hint 1-52: The changes look good to me!

The modifications to the footer button order improve the user experience by guiding users to the primary action. The code changes are clean and isolated. Pending verification of design consistency, this PR is approved.


14-14: Verify the primary button placement aligns with design guidelines.

The primary "确定" (Confirm) button is now placed before the secondary "取消" (Cancel) button. While this change emphasizes the primary action, it's important to ensure this placement aligns with the project's design guidelines and user expectations for consistency across the application.

Run the following script to check the button order in other dialog components:

examples/sites/demos/pc/app/dialog-box/right-dialog.vue (1)

14-14: LGTM!

The change to make the "确定" (Confirm) button a primary button type is a good improvement. It clearly distinguishes the primary action from the secondary "取消" (Cancel) button, enhancing the user experience and interaction clarity.

packages/theme/src/dialog-box/vars.less (5)

100-100: Confirm the impact of increasing the drawer body's left and right padding.

Changing the drawer body's left and right padding from 0px to var(--ti-common-space-8x) will increase the spacing around the content. Verify that this change does not introduce any layout issues or inconsistencies, particularly in scenarios where the drawer is used with fixed-width elements or alongside other components.

To confirm the impact, run the following script:

#!/bin/bash
# Description: Verify the usage of `--ti-dialog-box-drawer-body-padding-left` and 
#              `--ti-dialog-box-drawer-body-padding-right` in the codebase.

# Test 1: Search for the variable usage in Less files.
#         Expect: Occurrences in stylesheets that define the drawer body padding.
rg --type less -C 5 $'--ti-dialog-box-drawer-body-padding-(left|right)'

# Test 2: Search for the `ti-dialog-box-drawer-body` class usage in HTML/Vue files.
#         Expect: Occurrences in components that apply the drawer body styles.
rg --type html -C 5 $'ti-dialog-box-drawer-body'
rg --type vue -C 5 $'ti-dialog-box-drawer-body'

Also applies to: 106-106


112-112: Verify the alignment of the drawer footer with the updated top padding.

Modifying the drawer footer's top padding from var(--ti-common-space-5x, 20px) to var(--ti-common-space-6x) may affect the vertical alignment and spacing of the footer content. Ensure that this change maintains a balanced and visually pleasing layout, especially when the footer contains multiple elements or varying heights.

To verify the alignment, run the following script:

#!/bin/bash
# Description: Verify the usage of `--ti-dialog-box-drawer-footer-padding-top` in the codebase.

# Test 1: Search for the variable usage in Less files.
#         Expect: Occurrences in stylesheets that define the drawer footer top padding.
rg --type less -C 5 $'--ti-dialog-box-drawer-footer-padding-top'

# Test 2: Search for the `ti-dialog-box-drawer-footer` class usage in HTML/Vue files.
#         Expect: Occurrences in components that apply the drawer footer styles.
rg --type html -C 5 $'ti-dialog-box-drawer-footer'  
rg --type vue -C 5 $'ti-dialog-box-drawer-footer'

124-124: Confirm the consistency of divider colors in the dialog box and drawer.

The new variable --ti-dialog-box-drawer-divider-border-color is set to var(--ti-common-color-line-dividing), while the existing --ti-dialog-box-header-divider-border-color is changed from transparent to var(--ti-common-color-transparent). Verify that these changes result in a consistent and intentional application of divider colors across the dialog box and drawer components.

To confirm the consistency, run the following script:

#!/bin/bash
# Description: Verify the usage of `--ti-dialog-box-drawer-divider-border-color` and
#              `--ti-dialog-box-header-divider-border-color` in the codebase.

# Test 1: Search for the variable declarations.
#         Expect: One occurrence each in `packages/theme/src/dialog-box/vars.less`.
rg --type less -C 5 $'--ti-dialog-box-drawer-divider-border-color'
rg --type less -C 5 $'--ti-dialog-box-header-divider-border-color'

# Test 2: Search for the usage of relevant classes in HTML/Vue files.
#         Expect: Occurrences in components that apply the divider styles.
rg --type html -C 5 $'ti-dialog-box-drawer-divider'
rg --type vue -C 5 $'ti-dialog-box-drawer-divider'
rg --type html -C 5 $'ti-dialog-box-header-divider'  
rg --type vue -C 5 $'ti-dialog-box-header-divider'

Also applies to: 126-126


70-70: Verify the impact of changing the dialog box body text color.

Changing the dialog box body text color from primary to secondary may affect the readability and visual hierarchy of the content. Ensure that the secondary color provides sufficient contrast against the background and aligns with the overall design guidelines.

To verify the impact, run the following script:


72-72: Ensure consistency in text color between the dialog box body and drawer body.

Introducing a new variable --ti-dialog-box-drawer-body-color with the primary text color may lead to inconsistency with the dialog box body, which now uses the secondary text color. Consider aligning the text colors for a cohesive user experience, unless there's a specific design requirement for differentiating them.

To verify the consistency, run the following script:

packages/theme/src/dialog-box/index.less (3)

181-181: Verify the header border color change in the design.

The header's border color has been updated from var(--ti-dialog-box-drawer-divider-border-color) to var(--ti-dialog-box-head-divider-border-color). This change indicates a potential shift in the design intent or a correction to ensure that the header's styling is distinct from the body and footer.

Please confirm with the design team if this change aligns with their vision for the dialog box header.


194-195: LGTM! The body styling changes enhance visual clarity.

The changes to the .dialog-box__body class styling are beneficial:

  • Setting the text color to var(--ti-dialog-box-drawer-body-color) enhances the visual distinction of the body content.
  • Adding a border to the bottom of the body section using var(--ti-dialog-box-drawer-divider-border-color) aligns with the overall design consistency.

These modifications improve the clarity and visual hierarchy of the dialog box body.


Line range hint 1-298: Consider adding or updating tests.

The PR checklist indicates that tests have not been added for these changes. It's important to ensure proper test coverage, especially when modifying component styling.

Please consider:

  • Adding new tests to verify the expected visual appearance and behavior of the modified dialog box components.
  • Updating existing tests if the changes impact any current test cases.

This will help maintain the stability and reliability of the dialog box component.

padding: var(--ti-dialog-box-drawer-body-padding-top) var(--ti-dialog-box-drawer-body-padding-right)
var(--ti-dialog-box-drawer-body-padding-bottom) var(--ti-dialog-box-drawer-body-padding-left);
}
}
Copy link

Choose a reason for hiding this comment

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

Verify closing curly brace indentation.

The closing curly brace for the .dialog-box__body rule appears to be indented incorrectly. It should align with the opening curly brace of the rule.

Apply this diff to fix the indentation:

-      }
+    }
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
}
}

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.

2 participants