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(tooltip): refactor tooltip for react #831

Merged
merged 1 commit into from Nov 29, 2021
Merged

feat(tooltip): refactor tooltip for react #831

merged 1 commit into from Nov 29, 2021

Conversation

lijinke666
Copy link
Member

@lijinke666 lijinke666 commented Nov 27, 2021

👀 PR includes

✨ Feature

  • New feature

🎨 Enhance

  • Code style optimization
  • Refactoring
  • Change the UI
  • Improve the performance
  • Type optimization

🐛 Bugfix

  • Solve the issue and close #0

🔧 Chore

  • Test case
  • Docs / demos update
  • CI / workflow
  • Release version
  • Other ()

📝 Description

*. 重构 react tooltip, 和 core 层 解耦, 去除 getTooltipComponent 参数
*. 重构 tooltip 参数配置, 之前的 api 设计太绕了, 其实 tooltipComponent 和 element 就是一个概念

  • 优先级:showTooltip(方法级) content => tooltip.content (配置级) => 默认 content
const s2Options = {
  tooltip: {
-   tooltipComponent: ""
+   content: ""
  }
}
const s2 = new PivotSheet()

- s2.showTooltip({ element: "" })
+ s2.showTooltip({ content: "" })
- s2.tooltip.show({ element: "" })
+ s2.tooltip.show({ content: "" })

*. 新增 s2.tooltip.renderTooltip() 用于继承 BaseTooltip 自定义 tooltip (react 层采用这种方式解耦)

import { BaseTooltip, SpreadSheet } from '@antv/s2';

export class CustomTooltip extends BaseTooltip {
  constructor(spreadsheet: SpreadSheet) {
    super(spreadsheet);
  }

  show(){}
  hide(){}
  destroy(){}
  renderContent(){}
}


const s2Options = {
  tooltip: {
    renderTooltip: (s2) => new CustomTooltip(s2)
  }
}

*. 修复 s2.showTooltip({ element: xx }) 和 s2.tooltip.show({element:xx}) 类型错误

不修复错误, 直接 @ts-ignore 太偷懒了吧 :)
image

*. 修复 组件卸载时 tooltip 报错
*. 修复 组件 tooltip 只能点击一次 close #828
image

*. 修复 组件 通过实例 s2.tooltip.show 或者 s2.showTooltip 方式 调用 tootip 报错 close #813
*. 修复 单元格级别的tooltip 无法显示
*. 修复 s2Options.tooltip.content: '' s2.tooltip.show({ content: '' }) 但是 依然显示 tooltip 内容

其他:

  1. 将 sheet entry demo 移动到 react playground 中
  2. 优化 issue template
    image

TODO:

  • 各种场景的测试

🔗 Related issue link

🔍 Self Check before Merge

  • Add or update relevant Docs.
  • Add or update relevant Demos.
  • Add or update relevant TypeScript definitions.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2021

🎊 PR Preview has been successfully built and deployed to https://s2-preview-pr-831.surge.sh

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2021

Size Change: +70 B (0%)

Total Size: 184 kB

Filename Size Change
./packages/s2-core/dist/index.min.js 112 kB -2 B (0%)
./packages/s2-react/dist/index.min.js 67.9 kB +72 B (0%)
ℹ️ View Unchanged
Filename Size
./packages/s2-core/dist/style.min.css 356 B
./packages/s2-react/dist/style.min.css 3.1 kB

compressed-size-action

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2021

你好, @lijinke666 CI 执行失败, 请点击 [Details] 按钮查看, 并根据日志修复。

Hello, @lijinke666 CI run failed, please click the [Details] button try to fixed it.

@github-actions github-actions bot added the 🚨 test failed 单元测试挂了 label Nov 27, 2021
@lijinke666 lijinke666 changed the title [WIP] refactor(tooltip): custom tooltip for react [WIP] feat(tooltip): refactor tooltip for react Nov 27, 2021
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

packages/s2-react/playground/sheet-entry.tsx Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging 0f3b2b6 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lijinke666 lijinke666 changed the title [WIP] feat(tooltip): refactor tooltip for react feat(tooltip): refactor tooltip for react Nov 29, 2021
@github-actions github-actions bot added the pr(feature) new feature label Nov 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging ef32427 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging d81d11e into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

stone-lyl
stone-lyl previously approved these changes Nov 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging 857182e into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging b5e9e98 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging cea299e into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@xingwanying
Copy link
Member

单测挂了

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging f222754 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging 06e0645 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #831 (cb046d0) into master (c4413c9) will increase coverage by 0.38%.
The diff coverage is 71.42%.

❗ Current head cb046d0 differs from pull request most recent head 594e398. Consider uploading reports for the commit 594e398 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #831      +/-   ##
==========================================
+ Coverage   70.86%   71.24%   +0.38%     
==========================================
  Files         166      167       +1     
  Lines       11541    11550       +9     
  Branches     2702     2701       -1     
==========================================
+ Hits         8178     8229      +51     
+ Misses       2216     2182      -34     
+ Partials     1147     1139       -8     
Impacted Files Coverage Δ
packages/s2-core/src/utils/tooltip.ts 43.97% <ø> (ø)
packages/s2-react/src/utils/index.ts 67.74% <ø> (-1.49%) ⬇️
...kages/s2-core/src/utils/interaction/merge-cells.ts 70.62% <33.33%> (+26.34%) ⬆️
packages/s2-core/src/sheet-type/spread-sheet.ts 80.63% <80.00%> (ø)
packages/s2-core/src/ui/tooltip/index.ts 88.05% <85.71%> (+3.31%) ⬆️
packages/s2-react/src/common/constant/options.ts 95.65% <100.00%> (+0.41%) ⬆️
...kages/s2-react/src/components/tooltip/constants.ts 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4413c9...594e398. Read the comment docs.

@github-actions github-actions bot removed the 🚨 test failed 单元测试挂了 label Nov 29, 2021
Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

packages/s2-react/playground/sheet-entry.tsx Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging cb046d0 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

stone-lyl
stone-lyl previously approved these changes Nov 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2021

This pull request introduces 1 alert when merging 594e398 into f40f70d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Copy link
Member

@stone-lyl stone-lyl left a comment

Choose a reason for hiding this comment

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

优先级:showTooltip(方法级) content => tooltip.content (配置级) => 默认 content
和 renderTooltip

@lijinke666 lijinke666 merged commit 3e57279 into master Nov 29, 2021
@lijinke666 lijinke666 deleted the fix-tooltip branch November 29, 2021 12:11
@github-actions
Copy link
Contributor

🎉 This PR is included in version @antv/s2-v1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version @antv/s2-react-v1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version @antv/s2-v1.0.0-beta.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version @antv/s2-react-v1.0.0-beta.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

wjgogogo pushed a commit that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants