aralejs / aralejs.github.io

开放、简单、易用的前端基础类库
http://aralejs.github.io
MIT License
1.37k stars 321 forks source link

overlay 再次 review 后的建议 #73

Closed lifesinger closed 12 years ago

lifesinger commented 12 years ago

1). gjslint src/*.js 检查还有 10 处错误

2). position 属性的中的 baseXY 和 selfXY 的参数设计感觉真心不错,赞。

3). onChangeXxx 在调用时,默认会传入 val(当前值)、prev(改变之前的值)、key(属性的key值),因此后面的 onChangeXxx 等都可以精简,我顺手修改了下。

4). _onChangeClassName 应该先 remove 之前的,然后再添加新的。

5). _onChangePosition 和 setPosition 我重构了下,接收一个参数感觉更方便使用。

6). css 方法里,对 this.element && 判断没必要,this.element 肯定有的。

7). setup 里,this.before('show', shim.sync, shim); 有问题,按照 shim 目前的逻辑,应该是 this.after('show', shim.sync, shim); 才正确,因为 before show 先,this.element 还处于隐藏状态,这时同步 shim, shim 会保持 hidden 状态。这里需要和贯高讨论下,之前我也有这个误用。可能得需要 iframe-shim 提供 show / hide 方法,这样我们就可以按照直觉写成:

this.before('show', shim.show, shim); // 在 overlay 显示前,先显示 shim
this.after('hide', shim.hide, shim); // 在 overlay 隐藏后,再隐藏 shim

8). render 渲染 DOM 结构那几行代码,与下面的 _onChangeXxx 存在明显的重复。这里可以用一句话代替:

this.change(); // 让在 initialize 时 set 的有变化的属性生效。

对于 zIndex,由于有初始值,因此需要在 setup 里设置好:

this.element.css('zIndex', val);

9). _onChangeId 里面,如果 id 为空的判断,感觉没必要。

10). render 调用后,是否默认就应该显示出来?感觉 render().show() 怪怪的。

有些已经顺手修改了,有些还没有,详见:

https://github.com/alipay/arale/commit/8fb352b70337fec8c888a861812f51a32a4a74db#diff-2

改完后,貌似 example 有问题了,悲催,明天再讨论。

11). PS:test spec 还是有必要的,以保证能彼此大胆的修改代码。

lepture commented 12 years ago

早点休息呀。 :smile:

lifesinger commented 12 years ago

Overlay 的代码看起来比较漂亮了,这比看美女还让人兴奋呀。

不过的确要睡觉了,晚安各位!

antife-yinyue commented 12 years ago

瞻仰下“美女”

antife-yinyue commented 12 years ago

@lepture 表情怎么打上去的?

antife-yinyue commented 12 years ago

L49 还原样式?

L78 方法名是“是否可见”,但代码是“是否不可见”

lifesinger commented 12 years ago

@jsw0528 感谢也来 review

L49 笔误

L78 方法名有问题,今天讨论下

lifesinger commented 12 years ago

仔细再看了下 YUI3 和 KISSY 的 Overlay,可能还得做以下调整:

  1. width 和 height 应该是 widget 的属性,直接移动到 widget 上
  2. Overlay 的核心是 positionable 和 stackable, 也就是支持 定位 和 层叠,因此 Overlay 提供的属性需要包括 postion 和 zIndex
  3. 在 YUI3 里,Overlay 提供的属性有:
x/y/xy/align/centered
zIndex
shim
constrain

headerContent
bodyContent
footerContent
fillHeight

对我们来说,需要考虑增加的是:centered , 以后需要考虑 constrain

  1. KISSY 的 Overlay 有点职责不清,把 closable 等属性也搞进来了。不妥当。
lifesinger commented 12 years ago

Mask 应该是 Overlay 的子类,而且是单例形式,第一个反应的接口应该是:

var mask  =Mask.getInstance(config) 或 new Mask(config) 两者等价,都是返回单例
然后 overlay 上的方法和属性,mask 都有。 
lepture commented 12 years ago

@jsw0528 这个是 emoji 呀。这样打出来:

更多请看 http://www.emoji-cheat-sheet.com/

lifesinger commented 12 years ago

setPosition() 是否有必要

antife-yinyue commented 12 years ago

@lepture https://github.com/jsw0528/rails_emoji :wink2:

lifesinger commented 12 years ago

@afc163 attrs 里,position 改成 align 了? 明天讨论下。我暂时修改回 position 了,不然 onChange 事件无效。

lepture commented 12 years ago

@jsw0528 ruby 的有一个 md_emoji 来着。其实就是很简单的一个 snippet ,感觉没什么必要建一个 gem 呀。

很早以前写的 https://gist.github.com/2011858

antife-yinyue commented 12 years ago

@lepture 嗯,我是在练习 Gem 打包来着〜 md_emoji 他是 render 成 markdown 的,不过他的正则有 bug,发现我跟你用的正则一样,哈哈〜 打包成为 gem 的好处是可以傻瓜式使用〜

lifesinger commented 12 years ago

绝大部分已完成重构,关闭之。