feat: add plugin tooltip option prefixCls #7512
Conversation
Summary of ChangesHello @XiaoMing-xmg666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the tooltip plugin by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a prefixCls option to the tooltip plugin, allowing for customization of the CSS class prefix. The changes span the plugin's options, constructor, and style applications, and are supported by new tests and updated documentation. My main feedback is on the plugin's constructor, where the current option-merging logic could unintentionally override crucial default styles. I've suggested a more robust merging strategy to prevent this potential issue.
| const combineOptions = Object.assign( | ||
| { | ||
| style: { | ||
| [`.${options.prefixCls || ''}tooltip`]: { | ||
| visibility: 'hidden', | ||
| }, | ||
| }, | ||
| }, | ||
| Tooltip.defaultOptions, | ||
| options, | ||
| ); | ||
| super(context, combineOptions); |
There was a problem hiding this comment.
The current implementation for combining options might lead to issues. Object.assign performs a shallow merge. If a user provides a style object in the options, it will completely overwrite the default style that sets visibility: 'hidden'. This could cause the tooltip to be visible by default, which is likely not the intended behavior and a regression for users customizing styles.
To fix this, I recommend using a deep merge for the options, especially for the style property. This ensures that user-provided styles are merged with the default styles, preserving the essential visibility: 'hidden' rule. You'll need to import deepMix from @antv/util at the top of the file.
| const combineOptions = Object.assign( | |
| { | |
| style: { | |
| [`.${options.prefixCls || ''}tooltip`]: { | |
| visibility: 'hidden', | |
| }, | |
| }, | |
| }, | |
| Tooltip.defaultOptions, | |
| options, | |
| ); | |
| super(context, combineOptions); | |
| const defaultStyledOptions = { | |
| style: { | |
| [`.${options.prefixCls || ''}tooltip`]: { | |
| visibility: 'hidden', | |
| }, | |
| }, | |
| }; | |
| const finalOptions = deepMix({}, defaultStyledOptions, Tooltip.defaultOptions, options); | |
| super(context, finalOptions); |
There was a problem hiding this comment.
@XiaoMing-xmg666 这里得改下 Object.assign 可能会意外覆盖 style
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v5 #7512 +/- ##
=======================================
Coverage 95.59% 95.59%
=======================================
Files 188 188
Lines 9888 9892 +4
Branches 2134 2137 +3
=======================================
+ Hits 9452 9456 +4
+ Misses 436 404 -32
- Partials 0 32 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| template: { | ||
| prefixCls: this.options.prefixCls || '', | ||
| }, |
fixes: #7153
