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

Table: fix setCurrentRow method to cancel the selection of rows #16776

Closed
wants to merge 4 commits into from

Conversation

a631807682
Copy link
Contributor

@a631807682 a631807682 commented Jul 29, 2019

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide (中文 | English | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer relative issues for you PR.

@element-bot
Copy link
Member

element-bot commented Jul 29, 2019

Deploy preview for element ready!

Built with commit e084dbf

https://deploy-preview-16776--element.netlify.com

@ziyoung ziyoung added this to the 2.12.0 milestone Jul 31, 2019
@ziyoung ziyoung self-assigned this Jul 31, 2019
@@ -62,6 +62,8 @@ export default {
}
} else if (_currentRowKey) {
this.setCurrentRowByKey(_currentRowKey);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

目前 updateCurrentRow 的作用有两个:

  • ①当 data 改变时,尝试更新 currentRow 的数据。
  • ②重新设置 currentRow。

当 row 为 null 时,进了逻辑①。这显然是不符合预期的。


你现在的修改可以暂时解决问题,但是这种情况下应该触发 current-change 事件的。
可以再修改一下,可以把 updateCurrentRow 中的 ②重新设置 currentRow功能抽象出一个单独的函数来解决这个问题。

Copy link
Contributor

@ziyoung ziyoung Aug 1, 2019

Choose a reason for hiding this comment

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

@a631807682 把 updateCurrentRow 的逻辑 ② 去掉,然后提取出一个函数。
然后 setCurrentRow 执行的时候调用 store 中这个新方法。

setCurrentRow(states, row) {
    // this.updateCurrentRow(row);
    // ...
  }

@a631807682
Copy link
Contributor Author

@ziyoung Avatar的单元测试不通过 https://travis-ci.org/ElemeFE/element/jobs/566305923#L1378
关于这个 #16624 问题,有没有考虑通过data-uri或者karmaproxy方式替换目前单元测试中的所有外部链接?

@ziyoung
Copy link
Contributor

ziyoung commented Aug 1, 2019

@ziyoung Avatar的单元测试不通过 https://travis-ci.org/ElemeFE/element/jobs/566305923#L1378
关于这个 #16624 问题,有没有考虑通过data-uri或者karmaproxy方式替换目前单元测试中的所有外部链接?

改成 data-uri 这个需求提交新的 pr 吧。

@a631807682 a631807682 closed this Aug 1, 2019
@ziyoung
Copy link
Contributor

ziyoung commented Aug 1, 2019

@a631807682 咋突然关闭了?

@a631807682 a631807682 reopened this Aug 1, 2019
@a631807682
Copy link
Contributor Author

@ziyoung 提新的不需要关掉吗...有点尴尬我不会

@ziyoung
Copy link
Contributor

ziyoung commented Aug 1, 2019

@a631807682 你这个 pr 是修复 setCurrentRow 的 bug。如果要修复其他的 bug,再提交新的 pr。

@a631807682
Copy link
Contributor Author

@ziyoung #16847 单测那个已经提了

},

updateCurrentRow(currentRow) {
const { states, table } = this;
const { states } = this;
const { rowKey, _currentRowKey } = states;
// data 为 null 时,结构时的默认值会被忽略
const data = states.data || [];
const oldCurrentRow = states.currentRow;

if (currentRow) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这个 if 里面的代码可以删掉了,移到一个单独的函数中。
修改 store/index.js 中 setCurrentRow,换成你新添加的这个函数。

@ziyoung
Copy link
Contributor

ziyoung commented Aug 1, 2019

@ziyoung #16847 单测那个已经提了

嗯,我已经合并了。拉取最新的代码,rebase 一下再提交。

@a631807682
Copy link
Contributor Author

@ziyoung #16847 单测那个已经提了

嗯,我已经合并了。拉取最新的代码,rebase 一下再提交。

好的

@a631807682
Copy link
Contributor Author

完蛋,改了commit顺序变这样了。。。别理我我再改回去。。。

@ziyoung
Copy link
Contributor

ziyoung commented Aug 2, 2019

现在这一块的逻辑不好理清楚了。在 #15983 中为了修复 bug,修改了 updateCurrentRow 的实现。把代码给搞复杂了。看来 15983 的修改是有问题的。

@ziyoung
Copy link
Contributor

ziyoung commented Aug 2, 2019

想了想得把 15983 引入部分代码给撤销掉,恢复到之前 updateCurrentRow 的实现,然后新增一个 setCurrentRow 方法专门用来重新设置 currentRow。


不知道这块逻辑你有没有理清?如果没理清的话,我来改好咯(我的锅😅)。

@a631807682
Copy link
Contributor Author

@ziyoung 你来改吧,我不清楚这块逻辑。:smile:

@a631807682 a631807682 closed this Aug 2, 2019
@a631807682 a631807682 deleted the issue-16767 branch September 25, 2019 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment