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.

Sign up for GitHub

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

Jump to bottom

chore(refactor): init merge cell service #1018

Closed
wants to merge 4 commits into from
Closed

chore(refactor): init merge cell service #1018

wants to merge 4 commits into from

Conversation

Gggpound
Copy link
Contributor

@Gggpound Gggpound commented Jan 5, 2024

No description provided.

@Gggpound Gggpound marked this pull request as draft January 5, 2024 09:47
@Gggpound Gggpound force-pushed the feat-refactor-merged-cells-0105 branch from ce1d8b5 to 7d1e8e0 Compare January 5, 2024 09:49
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2024 •
edited

View Deployment

📑 Examples 📚 Storybook
🔗 Preview link 🔗 Preview link

@Gggpound Gggpound force-pushed the feat-refactor-merged-cells-0105 branch from 7d1e8e0 to 2084ba6 Compare January 8, 2024 06:40
@Gggpound Gggpound force-pushed the feat-refactor-merged-cells-0105 branch from 2084ba6 to 1ba99cd Compare January 22, 2024 06:49
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 22, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

38.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@codecov Codecov
Copy link

codecov bot commented Jan 22, 2024 •
edited

Codecov Report

Attention: Patch coverage is 63.13364% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 29.52%. Comparing base (ff8cfe7) to head (1ba99cd).
Report is 170 commits behind head on dev.

Files Patch % Lines
...eets/src/services/merge-cell/merge-cell.service.ts 30.00% 31 Missing and 4 partials ⚠️
...ackages/sheets/src/controllers/merge-cell/utils.ts 25.00% 10 Missing and 2 partials ⚠️
...src/controllers/merge-cell/merge-cell.ref-range.ts 80.00% 6 Missing ⚠️
...ommands/delete-range-move-left-confirm.command .ts 0.00% 4 Missing and 1 partial ⚠️
...s/commands/delete-range-move-up-confirm.command.ts 0.00% 4 Missing and 1 partial ⚠️
...commands/insert-range-move-down-confirm.command.ts 0.00% 4 Missing and 1 partial ⚠️
...ommands/insert-range-move-right-confirm.command.ts 0.00% 4 Missing and 1 partial ⚠️
...kages/sheets-ui/src/controllers/clipboard/utils.ts 72.72% 3 Missing ⚠️
.../sheets-ui/src/controllers/auto-fill.controller.ts 81.81% 1 Missing and 1 partial ⚠️
...ommands/commands/remove-worksheet-merge.command.ts 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##              dev    #1018   +/-   ##
=======================================
  Coverage   29.51%   29.52%           
=======================================
  Files         828      830    +2     
  Lines       46982    47050   +68     
  Branches     9629     9631    +2     
=======================================
+ Hits        13868    13892   +24     
- Misses      28903    28943   +40     
- Partials     4211     4215    +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gggpound Gggpound marked this pull request as ready for review January 22, 2024 07:01
@Gggpound Gggpound added the qa:untested This PR is ready to be tested label Jan 22, 2024
wzhudev
wzhudev reviewed Jan 22, 2024
View reviewed changes
packages/sheets/src/services/merge-cell/merge-cell.service.ts
);
}

getCellInfoInfo(unitID: string, subUnitId: string, row: number, col: number): ISelectionCell {
Copy link
Member

Choose a reason for hiding this comment

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

这个 API 名字是啥意思？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

迁移过来的,会拿到当前单元格的信息（包含合并单元格相关的属性。）

wzhudev
wzhudev requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

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

No context is provided for why this refactor is necessary and what is actually done. Unable to provide review opinions.

packages/core/src/sheets/worksheet.ts
* @deprecated use merge-cell-service
* @return {*} {IRange[]}
* @memberof Worksheet
*/
getMergeData(): IRange[] {
Copy link
Member

Choose a reason for hiding this comment

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

为什么要把这个 API 去掉？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个属于 model ，业务使用应该统一归属到 service .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

目前渲染层那边没有接入 di，所以这个 api 没办法完全去掉.

Copy link
Member

Choose a reason for hiding this comment

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

这个属于 model ，业务使用应该统一归属到 service .

为什么获取合并单元格算作所谓的“业务使用”，获取单元格内容不算？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我觉得拿单元格内容也应该走 service 拦一层的...要弱化 worksheet 这个概念, 本身 worksheet 算 snapshot。
对外提供 api 又是走的 facade（基于 service 进行的 api 组装）,

@zhaolixin7 zhaolixin7 added qa:verified This PR has already by verified by a QA and is considered good enough to be merge and removed qa:untested This PR is ready to be tested qa:verified This PR has already by verified by a QA and is considered good enough to be merge labels Mar 5, 2024
@Gggpound Gggpound closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wzhudev wzhudev wzhudev requested changes

@Dushusir Dushusir Dushusir approved these changes

@DR-Univer DR-Univer Awaiting requested review from DR-Univer DR-Univer is a code owner

@yuhongz yuhongz Awaiting requested review from yuhongz yuhongz is a code owner

@jikkai jikkai Awaiting requested review from jikkai jikkai is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Gggpound @wzhudev @Dushusir @zhaolixin7