switchable code review 20120606

[lifesinger]

1. 看了下 extra/utils.js 的调用处，感觉可以完全去掉，直接用 setTimeout 也挺精简的。buffer 也无必要。
2. 合理的空行可以让代码排版看起来更舒服。我调整了下 attrs 的，康辉再调整下其他的，最主要的调整：函数之间至少空一行哦，否则感觉好拥挤。还有 // 后面要有一空格，以及其他文档规范。
3. Line#19 - 37, 是 YUI 式的老风格了，很多变量是为了 YUI Compressor 的压缩写，现在可以考虑去掉。比如 DISPLAY，用到也就两处，可以内置掉。另外，由于我们是基于 jQuery 开发，因此 css(DISPLAY, val) 都不需要了，直接掉 show/hide 就好。（当初写 switchable 的时候，kissy 只有 dom / event）
4. 原来说的 markupType 的问题，是否可以简化为： 原来的默认结构，使用 data-role 来解决（和 navClass 和 contentClass 类似，但更具备 data-api 风格）：

    <div>
    <ul data-role="nav">...</ul>
    <div data-role="content"></div>
    </div>

原来的适度灵活结构，也由 data-role 来解决：

    <div>
    <ul><li data-role="trigger"></li></ul>
    <div><div data-role="panel"></div></div>
    </div>

完全自由结构，则可以直接给 triggers 和 panels 传入 selector 或 [] 来确定。

也就是说，我们的方式简化为两种：一种是通过属性 triggers 和 panels 来确定，另一种是通过 data-role 来确定。优先级是属性优先，data-api 的优先级是，小的优先（data-role="trigger"优先）。

data-role 的方式，最终还是转换成了 triggers 和 panels 的方式。康辉再想下，调整下。

1. activeTriggerClass 用 ui-switchable-active 更合适？尽量用长命名风格。
2. 用不去用检查传入的select是否符合规范？比如'.'的检查？这个不用了，还是要信任用户，否则要检查的东西太多。
3. initProps 方法里暴露的三个 property 感觉不妥，如果是给自己用的，加上下划线，否则是 public api，外部用户可调用。搜索了下代码，好像这三个 property 并不很常用，感觉没必要暴露出来，默认值更应该在 attrs 里描述清楚。
4. 对 $ 的覆盖不妥，应该与 Arale 的组件保持一致，而不是与 KISSY 的习惯保持一致。
5. setup 方法里，依次调用的是 initPanels, initTriggers, bindAction 等，但紧接着这四个方法的定义顺序与 setup 中的不一致，建议调整下，使得阅读代码时更通畅。
6. throw new Error(...) 可以简化为 throw 'xxx'
7. _cancelSwitchTimer 的实现，可以简化为 clearTimeout(switchTimer)（先把 utils.later 去掉）
8. _postCreate 一眼看不错里面进行什么操作，是否改名叫 _installPlugins ？里面 willSwitch 的操作，应该是 attr 里自动处理好了，不需要手动调 _onChangeActiveIndex.
9. _onChangeActiveIndex 应该改成 _onRenderActiveIndex
10. prev 和 next 应该只需要 set('activeIndex', 要激活的 index) 就行，现在好奇怪
11. _parent 方法的封装不错
12. 插件的设计感觉还是有问题，比如初始配置里，如果 autoplay 为 false，目前的设计，不会插入 Autoplay 的成员，但用户很可能只是暂停一段时间（比如让首屏停留久点），然后才调用 tabs.start() 或 tabs.set('autoplay', true) 来开始轮播，目前的插件的设计，太依赖初始化阶段。感觉还是应该放在 Switchable 类上，是类的 extension，而不是对象的 plugin
13. data-wiget-config 昨晚我仔细看了几个类似的方案，倾向于直接用单个，比如 data-effect="scrollx", data-delay="200", 这样有个好处，在 _autoRender(element, config, dataset) 第二个参数就是用户写在 element 的配置，比如：

<div data-widget="tabs" data-triggers="li" data-panels="div.panel" data-active-index="2" data-effect="fade">
...
</div>

调用 Switchable 的 autoRender 时，第二个参数是：

autoRender = function(element, config, dataset) {
   config 是 {  triggers: 'li', panels: 'div.panel', activeIndex: "2", effect: "fade"  }
}

这样，调用 autoRender，等价于 new Tabs({ triggers: 'li', panels: 'div.panel', activeIndex: "2", effect: "fade" })

data-api 的命名规范也有了，就是 data-attr-name 必须是 attr 的连字符形式的名称

甚至 autoRender 的第一个参数 element 也没必要了，直接放在 config 里：{ element: element, triggers: ...}

康辉再看下，越发觉得这样约定很好。

1. 最后这个注释应该无效吧：

// _ 代表保护方法，可以被子类覆盖。
// __ 代表私有方法，子类不可以覆盖。

删除之

1. autorender.js 可以去掉，可以借鉴 widget/src/auto-render.js, 将 我们的 autoRender 逻辑独立出去，写在 auto-render 文件里
2. 其他无问题。写得挺辛苦的，康辉不容易呀，我们继续努力。另外，有个小建议：可以尝试用 Intellj IDEA 或 Eclipse 等 IDE 来写代码，用 Vim 写小代码还行，当代码行数太多，文件到达一定复杂度时，用 IDE 还是有很大好处，比如 目前 effects.js 文件里，有很多无效的变量，Effects 还重复 var 了，以及 switchable-spec.js 里，Tabs 拼错成了 Tabse, Carousel 也拼写错了，这些如果有 IDE 辅助，会轻松很多，用 Vim 的话，需要极大的耐心才能检查出来。

---

一起努力！

[leoner]

1, 2, 3, 5, 7, 9, 10, 11,12, 13, 17,18 已经修改完毕。

4. 这个需要在想下啊， 当时把nav,content转换为triggers,panels主要是方便在switchable里面减少判断，可以统一操作啊，如果增加nav,content的话，可能还要增加额外的逻辑判断

6 , 8 主要是想省去 dot + '**Class', 因为有很多操作都要手动添加 '.'

13. 已经修改，但是在初始化的时候没有调用此方法，需要玉伯看下是啥原因。

14. 主要是在prev和next中需要给switchTo传入方向这个参数，如果通过set('activeIndex')来做的话，无法把方向参数传递过去。

16. 因为这些插件有些是对实例方法的覆盖，也就是说是否启用插件会对具体的方法有影响，不过可以增加延迟加载插件啊，那是不是还需要一个动态卸载的？

19. 因为目前有些子类也有自己的autoRender所以目前暂时把autoRender保留在对应的文件中了。

20. IDE确实可以减少很多这种低级错误啊，下一步可以看看选用一个，主要是因为IDE速度有点慢啊，看来SSD硬盘不换不行了啊

[antife-yinyue]

willSwitch 看着有点耳熟〜

第 6 点很赞成，是我的一贯风格，东西是给你用的，不是给你折腾的，嗯〜

[lifesinger]

有点感冒，最近大脑有点昏沉。看了下源码，康辉好速度，大部分都重构好了。继续探讨几个问题：

---

4: navClass 和 contentClass 需不需要的问题。我的想法是，因为我们默认推崇的是 data-api，因此通过 navClass / contentClass 的方式来指定“配置”的方式，可以统一为用 data-api 来实现，比如，使用 class 的方式，需要：

<div data-widget="tabs" data-nav-class="xx-nav" data-content-class="xx-content">
<div class="xx-nav">...</div>
<div class="xx-content">...</div>
</div>

上面的 xx-nav / xx-content 得至少写两次。

但统一用我们默认推荐的 data-api 的话，只需要：

<div data-widget="tabs">
<div class="xx-nav" data-role="nav">...</div>
<div class="xx-content" data-role="content">...</div>
</div>

更简洁，也让 class 和 hook 更解耦了。

这样，我们只要告诉用户我们支持的 data-role 这个 api 就好，role 可取的值为 nav / content / trigger / panel 其他的都是 this.element 上的 data-attr-name 的方式，含义很好理解，并且可以让所有组件保持一致

建议 attrs 里去掉 navClass / contentClass 配置

---

6 和 8: 去掉 navClass / contentClass 配置后，需要 DOT 的场景就很少了。最主要的是，不要破坏 this.$ 的一致性，比如在其他组件中，可以通过 this.$('li') 来获取子元素，但在 switchable 里，就出问题了，一致性没保证。

---

13 我查了下 examples/tabs.html, 是因为没有掉 render 导致的，调用 render 时，才会调用 _onRenderXx. 现在已经好了。git pull 下。

---

14 direction 参数我记得只在 circular 开启时有用，但现在回想起来，当初这个有点过渡设计，是为了解决 从 len - 1 跳到 0 时的两种情况，当 direction 是 forward 时，是顺向滚动，是 barward 时，则反向滚动到 0，但实际上，switchable 在淘宝上线后，我记忆里从来没这类需要，也没有需求方 care 到这个细节。因此这里应该可以简化掉，去掉 direction，direction 的判断可简化为：如果 toIndex 大于 fromIndex, 则是顺的，反之反的。特例是，circular 打开时，从 len - 1 到 0 也是顺的，无反的情况。

---

16 plugin 这一块还是怪怪的，先这样。等有时间时我们再仔细想想如何优化。

---

19 autoRender 可以删掉了，widget 默认的应该够用了

---

20 IDE 还好的，速度不慢。用 Vim 需要功力很深很深很深，目前国内还没什么人能用好。

---

新的讨论点

21: activeIndex 目前有 setter，支持字符串和负值，感觉意义不大，交给用户处理就好。能少则少，早期不增加“可能的”使用场景支持，除非以后证明真有这个需求。

22: 康辉熟悉下 widget 的 auto-render 逻辑，熟悉后，将 switchable 相关代码重构下，应该可以简化不少。另外，我们不用支持 data-widget-config 这种形式，直接写成单个更好（更符合 data-api 标准，也更不容易出错）

一起努力，继续加油！

[leoner]

4 navClass的一个作用就是阶梯原来hasTriggers的作用，因为有些情况可以允许用户自定义生成trigger.这个时候必须有一个参数来表明是否需要生成。当时就借用这个参数了，如果配置了这个参数，而且页面没有发现这个元素的话，我们就自动生成。现在看来还是去掉navClass然后恢复hasTriggers这个参数，这样可能更加清晰。 但是由于自动生成的元素也需要class,这样的话，我们默认还是需要有一个属性来表明，我们自动创建的这个nav的class是什么？

4 contentClass也主要是为了给用户提供一个便利，可以让用户可以使用默认的配置，目前觉得还是统一下更好，既然提供了data-api，就让用户去用吧，这样整体的风格也很统一。

[leoner]

4 还是很纠结，就是关于自动生成trigger这块。现在有这几种方式

1. 我们是否明确提供hasTriggers这个参数？
2. 我们自动生成的nav，他的class如何配置进来？ 默认？

[lifesinger]

4: hasTriggers 当初也是设计过度，后来只遇到过一个需求：就是 triggers 存在时，想隐藏掉。按照我们目前的设计，只需要 triggers 传入空值就好。由 js 创建 triggers 的情况没遇到过-.- 因此 hasTriggers 属性也可以去掉了，保持精简

所以，navClass / contentClass / hasTriggers 都去掉，除非以后真的有强烈需求（感觉淘宝没有，支付宝也应该不会有，呵呵）

[leoner]

4 增加classPrefix和hasTriggers属性. 默认ui-switchable-nav 如果传入classPrefix 就是classPrefix-nav

[leoner]

上面讨论的已经修改完毕啊！