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(cell): border width issue #859

Merged
merged 10 commits into from
Dec 3, 2021
Merged

fix(cell): border width issue #859

merged 10 commits into from
Dec 3, 2021

Conversation

zxc0328
Copy link
Member

@zxc0328 zxc0328 commented Dec 2, 2021

👀 PR includes

📝 Description

修复了 data cell 边框渲染的粗细不一的问题。

顺便对 col/row/corner 的边框渲染逻辑做了抽取和统一。统一使用 getBorderPositionAndStyle 绘制。

🖼️ Screenshot

Before After
image image
image image
image image

🔗 Related issue link

close #426

@todo
Copy link

todo bot commented Dec 2, 2021

边框整体重构后需revisit

x: x + style / 2, // TODO: 边框整体重构后需revisit
y: y + style / 2,
width: width - style - 1,
height: height - style - 1,
};


This comment was generated by todo based on a TODO comment in c2262e9 in #859. cc @antvis.

@github-actions github-actions bot added the pr(fix) bug fix label Dec 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2021

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

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 2 alerts when merging c2262e9 into 025ec55 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2021

Size Change: +327 B (0%)

Total Size: 185 kB

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

compressed-size-action

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #859 (d6b3b08) into master (6f174fc) will increase coverage by 0.39%.
The diff coverage is 79.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
+ Coverage   71.26%   71.65%   +0.39%     
==========================================
  Files         167      169       +2     
  Lines       11574    11766     +192     
  Branches     2708     2750      +42     
==========================================
+ Hits         8248     8431     +183     
- Misses       2184     2185       +1     
- Partials     1142     1150       +8     
Impacted Files Coverage Δ
packages/s2-core/src/theme/palette/default.ts 100.00% <ø> (ø)
packages/s2-core/src/cell/table-series-cell.ts 63.15% <50.00%> (-8.03%) ⬇️
packages/s2-core/src/cell/corner-cell.ts 70.87% <63.63%> (+0.16%) ⬆️
packages/s2-core/src/facet/header/series-number.ts 55.03% <68.75%> (+2.57%) ⬆️
packages/s2-core/src/cell/table-corner-cell.ts 65.43% <70.00%> (-5.76%) ⬇️
packages/s2-core/src/cell/data-cell.ts 58.73% <83.33%> (+0.49%) ⬆️
packages/s2-core/src/cell/table-data-cell.ts 64.10% <83.33%> (+0.10%) ⬆️
packages/s2-core/src/cell/col-cell.ts 76.57% <87.50%> (+0.99%) ⬆️
packages/s2-core/src/cell/row-cell.ts 72.05% <92.30%> (-0.09%) ⬇️
packages/s2-core/src/cell/base-cell.ts 76.97% <100.00%> (ø)
... and 19 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 d1f4823...d6b3b08. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 2 alerts when merging 7dc1adb into 025ec55 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 2 alerts when merging 6ee4411 into 025ec55 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Dec 3, 2021

This pull request introduces 2 alerts when merging d6b3b08 into 025ec55 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@lijinke666 lijinke666 merged commit 114e7fc into master Dec 3, 2021
@lijinke666 lijinke666 deleted the fix-border-width branch December 3, 2021 05:56
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

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

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

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

The release is available on:

Your semantic-release bot 📦🚀

wjgogogo pushed a commit that referenced this pull request Oct 25, 2023
* fix: data cell border

* fix: data cell border issue

* feat: update test spec

* refactor: border draw call into one method

* refactor: series number node border

* fix: tree hierachy border

* refactor: combine border draw call

* feat: replace cell border

* feat: replace cell border

* feat: replace cell border
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Cell 边框渲染遮挡问题
2 participants