-
Notifications
You must be signed in to change notification settings - Fork 0
[package] Improve with .controlSections
#35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRに含まれている内容はよさそうです:+1:
Controlの再帰が残っているところがるので、このPR内で消せるとよさそうな気がしました!
package/src/Emaki/Control.elm
Outdated
| String (StringControl msg) | ||
| Bool (BoolControl msg) | ||
| Select (SelectControl msg) | ||
| Radio (RadioControl msg) | ||
| Counter (CounterControl msg) | ||
| BoolAndString (BoolAndStringControl msg) | ||
| List (List (Control msg)) | ||
| Field String (Control msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらのControlの再帰もできれば消したいのですが、こっちは消せなさそうですかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
消すためには、Control型の各Variantについて .label
プロパティを追加することになると思います。
現時点では Fieldに集約して凝集度を高めておくほうが若干スマートかなというところ。
もっとも、#34 に向けてこの方法を採用したい理由があれば、このPRで予め変更しておくようにしても良いです。
別案としてplayground関数側で常に付けるような方法も考えられますが、Control.Comment
にlabelを付ける可能性がほぼないので、この方法だと、別のところに冗長さが露出することになってしまう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
また、将来的にテキストフィールドなどは右カラムの幅いっぱいに入力欄を広げたい可能性が大きく、そのあたりのオプションをVariantそれぞれに対してありなし決めていくよりはFieldに集約しておくほうが変更に強いかな?といったところ。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「コントロールセクションたちというのは、コントロールをリストにしたもの」という定義を読んだ後に、そのコントロールの定義の中に「BoolControlはコントロールでBoolと呼ぶ、SelectControlはコントロールでSelectと呼ぶ、...」と定義が並んでいる中で、「文字列とコントロールはコントロールでフィールドと呼ぶ、文字列はコントロールでコメントと呼ぶ」という定義がきているような違和感があるんですよね。
前者は純粋に何かの値を操作するための表示で、後者は「操作するための表示」文書構造を与える構造な気がしていますね〜、これをデータ型で区別したいという意図です
すごくざっくりなコードのイメージだと
-- 値を操作するための表示がどんな種類があるかの定義
type Control
= Bool ...
| Select
| Radio
-- 右に表示するパネルにどんな見た目で表示できるかの定義
type alias ControlSection
= { sectionTitle : Maybe String
, controls : List { label = Maybe String, control = Control }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controlとcontrolの間に純粋なテキストノードを入れるのであれば、type Node = TextNode String | Control (Maybe String) Control
のようにしてsectionBody : List Node
にしてしまうのも良いかなという感じです。
改めてまとめると、現在のControlは「値をどうコントロールするかの表示」と「サイドパネルにどのような文書構造を持たせるか」が混ざってしまっているので、List (List (Control msg))をなくすのであればそこまでやりきりたいです!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2,8 +2,7 @@ module Emaki.Control exposing | |||
( Control | |||
, StringControl, BoolControl, SelectControl, RadioControl, CounterControl, BoolAndStringControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この辺はリネーム前の xxxProps が適切だったかもしれない。
このPRでは気にしないでおきます。
playgroundの
.props
プロパティを.controlSections
に変更し、Control
モジュールを簡素化しました。