gaogaotiantian / biliscope

Bilibili chrome extension to show uploader's stats
MIT License
594 stars 46 forks source link

Refactor function `initEvents` which in `UserProfileCard` #109

Closed F-park closed 10 months ago

F-park commented 10 months ago

使用局部变量 arrowUparrowDown 重构函数 initEvents

gaogaotiantian commented 10 months ago

这是为什么呢。?就是它的价值在哪里?我不是说这个不好啊,我觉得完全可以接受,但是为了这个feature把程序逻辑明显变得更复杂了。现在这个event会经常性地被开关,而哪里一个不小心就可能导致这个event被加两次或者开关搞错。这也不是不能接受,这个PR的核心问题是,这个cost/gain太不划算了。用一个增加了不小风险的实现,去做了一个收益非常低的corner case。

F-park commented 10 months ago

现在这个event会经常性地被开关,而哪里一个不小心就可能导致开关搞错。

现在这个 event 只有打开关闭备注框才会开关吧(其他的逻辑是和之前完全一样的,就是只在开始时 initEvents),而且也只有这个 feature 会使用到 createEventsremoveEvents 这两个函数吧,我觉得不会造成存在搞错的问题(实在不放心的话可以改这两个函数和 setEvents 的名字)。

而哪里一个不小心就可能导致这个event被加两次。这也不是不能接受,这个PR的核心问题是,这个cost/gain太不划算了。用一个增加了不小风险的实现,去做了一个收益非常低的corner case。

在浏览器控制台通过 getEventListeners 函数测试了一下,event 就不存在被加两次的情况(因为加进去的是同一个函数,疑似根本就没加进去或者被重写了),删除两次也是一样的,第二次删除不会发生任何事情。所以这个 feature 我认为是没有风险的,因为只有开和关两种状态。

gaogaotiantian commented 10 months ago

我依然是不喜欢这个改动,我觉得收益实在是太低了。我甚至不觉得不显示这个箭头是一个显著更好的状态,当然我也并不觉得它变差了。但是带来的复杂度上的cost是在维护上完全没必要接受的。

F-park commented 10 months ago

我依然是不喜欢这个改动,我觉得收益实在是太低了。我甚至不觉得不显示这个箭头是一个显著更好的状态,当然我也并不觉得它变差了。但是带来的复杂度上的cost是在维护上完全没必要接受的。

那就只重构函数不加新功能吧,其实目前的问题是备注框收回时卡片下面的位置变了,导致箭头按钮的 click 事件无法触发,之后再研究研究怎么修复。