gaogaotiantian / biliscope

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

修改ui显示问题 #61

Closed lovefyf928 closed 1 year ago

lovefyf928 commented 1 year ago

修复了如果备注输入框是显示状态再次点击biliscope-username-wrapper这个区域,输入框会闪烁的问题。 不知道这个问题是不是期待的行为,但是我感觉有点奇怪。

gaogaotiantian commented 1 year ago

一般来说,我们在一个PR里只改一个东西,而且要把PR的“实际改动”说明白。

你的这个PR改了若干东西,并且都没有说明原因。

比如我其他的大量event都是click触发的,你在这里给我偷偷改成了mousedown,这显然是不合适的。

left-click的部分,不是不行,但是你也没说明。我个人的感觉是这里不是很需要。

follow button那里,因为那个button设置了stopPropagation,所以不应该触发才对。这里你测试过么?是有效代码么?

包括你前面的preventDefault,是有意义代码么?prevent了什么呢?

这都是你应该说明的内容。

那实际上你真正和这个PR里说明的内容有关的是修改的471-474嘛,这个feature本身是ok的,就相当于是再点击用户名就收起来吧。

然后下面那个text的click event你是有意识地没改的是么?因为它理论上并不会在textarea出现的时候出现?

我的建议是,把你的无关改动都删了,只留下你PR描述内容内的改动,那个改动是ok的。

lovefyf928 commented 1 year ago

我是测试过的如果只留下741-474是不好使的 1、这个事件改为mousedown是因为事件触发顺序click会晚于blur。 2、left-click是因为其他的事件是click,click是左键触发。 3、follow button是click事件会晚于mousedown所以follow button里面的stopPropagation是不好使的 4、blur通过mousedown执行默认行为来触发,所以如果这边不preventDefault就会马上blur导致输入框永远打不开 5、text不会在textarea的时候出现

gaogaotiantian commented 1 year ago

这个理由听起来科学了很多。尽管我不太喜欢忽然出现一个mousedown的event,不过我想了一下这个可能是目前最好的解决方案了。

你可以把两个“不触发”的情况合并一下么?应该做blur的那个放到纯return的后面。

然后这个情况明显和其他的event是有一些关联的,所以稍微注释一下,解释一下为什么用到了mousedown以及为什么需要做一些看似无关的操作。

除此以外就没问题了,加上就可以merge了。

lovefyf928 commented 1 year ago

好的,我修改了并添加了注释

gaogaotiantian commented 1 year ago

感谢贡献!虽然代码量不多,但是是个挺有价值的修改。

lovefyf928 commented 1 year ago

谢谢指导

lovefyf928 commented 1 year ago

突然想起把blur放到纯return最后会导致如果在textarea显示的状态点击订阅按钮textarea不会直接隐藏,而是会等数据更新后才会消失

lovefyf928 commented 1 year ago

我重新提交一个pr将这个问题修复一下吧 @gaogaotiantian

gaogaotiantian commented 1 year ago

这里重新处理一下吧,不对那个wrapper做event绑定了,新建一个div,包括用户名,gender和level,然后对这个div做event绑定,直接把那个button排除出去。因为现在有了这个button,在这个button后面的空白点开备注其实是一个不那么直觉的事情了。

lovefyf928 commented 1 year ago

好的,我晚点修改好了再提交一个pr