Skip to content

Conversation

@zouawen
Copy link
Contributor

@zouawen zouawen commented Oct 5, 2025

Description

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Checklist

ℹ️ Check all checkboxes - this will indicate that you have done everything in accordance with the rules in CONTRIBUTING.

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs:dev command.
  • Run the tests with pnpm test.
  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • Style

    • Updated GitHub, Google, WeChat, QQ, and DingDing icons to new SVG variants across layouts, login UI, and demos for sharper, consistent visuals.
  • New Features

    • Added localization entries for third-party login labels (WeChat, QQ, GitHub, Google) in English and Chinese.

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2025

⚠️ No Changeset found

Latest commit: 67ee2be

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

Walkthrough

Icon components were migrated from Mdi* to Svg* across multiple UI files. Corresponding exports were removed from iconify and added under svg. Authentication locale entries were extended in en-US and zh-CN. No logic, control flow, or data handling changes were introduced.

Changes

Cohort / File(s) Summary of changes
App layouts: GitHub icon swap
apps/web-antd/src/layouts/basic.vue, apps/web-ele/src/layouts/basic.vue, apps/web-naive/src/layouts/basic.vue
Replaced MdiGithub with SvgGithubIcon (imports and usage).
Playground: icon updates
playground/src/layouts/basic.vue, playground/src/views/demos/features/icons/index.vue
Switched GitHub and other Iconify demos from Mdi* to Svg* components; kept MdiKeyboardEsc.
Auth UI: third-party icons
packages/effects/common-ui/src/ui/authentication/dingding-login.vue, packages/effects/common-ui/src/ui/authentication/third-party-login.vue
Replaced DingDing/WeChat/QQ/GitHub/Google icons from Mdi*/Ri* to Svg* components (imports and template).
Icons package: iconify removals
packages/icons/src/iconify/index.ts
Removed exports: MdiWechat, MdiGithub, MdiGoogle, MdiQqchat, RiDingding; kept MdiKeyboardEsc.
Icons package: svg additions
packages/icons/src/svg/index.ts
Added exports: SvgGithubIcon, SvgGoogleIcon, SvgQQChatIcon, SvgWeChatIcon, SvgDingDingIcon.
Locales: authentication labels
packages/locales/src/langs/en-US/authentication.json, packages/locales/src/langs/zh-CN/authentication.json
Added keys: weChat, qq, gitHub, google with translations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • feat: add dingding login #6567 — Introduced RiDingding and Dingding login usage previously; directly related to current removal of Ri/Mdi exports and switch to Svg* icons.

Suggested reviewers

  • mynetfan
  • anncwb
  • jinmao88
  • vince292007

Poem

I swap my stars for shiny SVGs ✨
Hopping through menus with whiskered glee,
QQ, WeChat, GitHub—icons agree,
DingDing bells ring in symmetry.
Locale carrots planted neatly,
A tidy patch—so crisp, so sweetly.
(_/)> 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description section contains only placeholder comments and lacks any actual summary of the implementation details, and the Type of change list has all options checked instead of removing irrelevant entries, so it does not meet the template requirements. Please replace the placeholder comments in the Description section with a clear summary of what was changed and why, and adjust the Type of change section by selecting only the appropriate entries and deleting the rest.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title directly describes the two main updates—switching login method icons to colored variants and adding tooltip text—so it accurately reflects the core changes in the pull request. It is related to the changeset and highlights the primary enhancements made to the login UI.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/locales/src/langs/en-US/authentication.json (1)

54-57: Consider using consistent key naming convention.

The new locale keys use mixed capitalization (weChat, gitHub) which is inconsistent with existing similar keys that use all lowercase (wechatLogin, qqLogin, githubLogin at lines 39-43). Consider using lowercase for consistency:

Apply this diff to align with the existing naming convention:

-  "weChat": "WeChat",
-  "qq": "QQ",
-  "gitHub": "GitHub",
-  "google": "Google",
+  "wechat": "WeChat",
+  "qq": "QQ",
+  "github": "GitHub",
+  "google": "Google",

Note: If you apply this change, ensure that all references to these keys in the codebase (e.g., in Vue components) are also updated accordingly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e03de5 and 67ee2be.

⛔ Files ignored due to path filters (5)
  • packages/icons/src/svg/icons/dingding.svg is excluded by !**/*.svg
  • packages/icons/src/svg/icons/github.svg is excluded by !**/*.svg
  • packages/icons/src/svg/icons/google.svg is excluded by !**/*.svg
  • packages/icons/src/svg/icons/qqchat.svg is excluded by !**/*.svg
  • packages/icons/src/svg/icons/wechat.svg is excluded by !**/*.svg
📒 Files selected for processing (11)
  • apps/web-antd/src/layouts/basic.vue (2 hunks)
  • apps/web-ele/src/layouts/basic.vue (2 hunks)
  • apps/web-naive/src/layouts/basic.vue (2 hunks)
  • packages/effects/common-ui/src/ui/authentication/dingding-login.vue (2 hunks)
  • packages/effects/common-ui/src/ui/authentication/third-party-login.vue (2 hunks)
  • packages/icons/src/iconify/index.ts (0 hunks)
  • packages/icons/src/svg/index.ts (2 hunks)
  • packages/locales/src/langs/en-US/authentication.json (1 hunks)
  • packages/locales/src/langs/zh-CN/authentication.json (1 hunks)
  • playground/src/layouts/basic.vue (2 hunks)
  • playground/src/views/demos/features/icons/index.vue (2 hunks)
💤 Files with no reviewable changes (1)
  • packages/icons/src/iconify/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/icons/src/svg/index.ts (1)
packages/@core/base/icons/src/create-icon.ts (1)
  • createIconifyIcon (14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Check (windows-latest)
  • GitHub Check: Test (ubuntu-latest)
  • GitHub Check: Lint (ubuntu-latest)
  • GitHub Check: Check (ubuntu-latest)
  • GitHub Check: Lint (windows-latest)
  • GitHub Check: Test (windows-latest)
  • GitHub Check: post-update (ubuntu-latest)
  • GitHub Check: post-update (windows-latest)
🔇 Additional comments (13)
packages/icons/src/svg/index.ts (2)

14-18: LGTM! Icon definitions follow the established pattern.

The new SVG icon constants are correctly defined using createIconifyIcon and follow the same pattern as existing icons.


29-34: LGTM! Exports are properly ordered.

The new icon exports are alphabetically ordered and follow the existing export conventions.

apps/web-antd/src/layouts/basic.vue (2)

9-9: LGTM! Icon import correctly updated.

The import has been properly updated from the old Mdi icon to the new SVG icon variant.


79-79: LGTM! Icon usage matches the import.

The icon component usage is consistent with the updated import.

apps/web-ele/src/layouts/basic.vue (1)

9-9: LGTM! Consistent icon replacement.

The GitHub icon has been correctly replaced with the SVG variant, matching the pattern used in other layout files.

Also applies to: 79-79

playground/src/layouts/basic.vue (1)

9-9: LGTM! Icon replacement matches other layouts.

The icon update is consistent with the changes made to other layout files (web-antd, web-ele, web-naive).

Also applies to: 92-92

packages/effects/common-ui/src/ui/authentication/dingding-login.vue (1)

4-4: LGTM! DingDing icon successfully migrated to SVG.

The icon replacement from RiDingding to SvgDingDingIcon is correct and follows the pattern used throughout this PR.

Also applies to: 99-99

apps/web-naive/src/layouts/basic.vue (1)

9-9: LGTM! All layout variants now use consistent SVG icons.

The icon update completes the migration across all layout variants (web-antd, web-ele, web-naive, playground).

Also applies to: 79-79

packages/effects/common-ui/src/ui/authentication/third-party-login.vue (2)

3-8: LGTM! Icon imports correctly updated.

All four third-party login icons have been properly migrated from Mdi* to Svg* variants:

  • MdiWechat → SvgWeChatIcon
  • MdiQqchat → SvgQQChatIcon
  • MdiGithub → SvgGithubIcon
  • MdiGoogle → SvgGoogleIcon

40-61: LGTM! Icon usages match the updated imports.

All icon component usages have been correctly updated to use the new SVG icon components. The tooltips (wechatLogin, qqLogin, githubLogin, googleLogin) were already present prior to this PR.

packages/locales/src/langs/zh-CN/authentication.json (1)

54-57: Confirm new localization keys are used

No occurrences of authentication.weChat, authentication.qq, authentication.gitHub, or authentication.google were found. Verify whether these are intended for future use or should be removed to prevent dead code.

packages/locales/src/langs/en-US/authentication.json (1)

54-57: No discrepancies found in zh-CN authentication.json zh-CN locale keys match en-US exactly.

playground/src/views/demos/features/icons/index.vue (1)

15-18: All authentication components use new Svg icons* Authentication components (including third-party-login.vue) now import and render SvgGithubIcon, SvgGoogleIcon, SvgQQChatIcon, and SvgWeChatIcon with no remaining Mdi* icon usage.

@jinmao88 jinmao88 merged commit 33306a5 into vbenjs:main Oct 5, 2025
28 of 30 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants