Skip to content

Add initial alpha value to preferences#284

Merged
smikitky merged 7 commits into
masterfrom
initial-alpha
Jun 16, 2022
Merged

Add initial alpha value to preferences#284
smikitky merged 7 commits into
masterfrom
initial-alpha

Conversation

@ttakenaga
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@smikitky smikitky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

お願いします

personalInfoView?: boolean;
theme?: string;
referenceLine?: boolean;
alpha?: number;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CS/DB 通じてのあらゆるユーザ設定を置く場所に alpha だけの設定名だと意味が伝わりません。initailAlphaForNewLabels くらい長い名称にしてください。

);
};

const InitialAlphaEditor: et.Editor<number | undefined> = props => {
Copy link
Copy Markdown
Member

@smikitky smikitky Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • value から毎回計算すればいいだけなので state は不要です。その場で計算できるものを state にしないというのは鉄則です。仮に state が必須だとしても newなんちゃら というネーミングはおかしいです。
  • コンポーネントの名前を AlphaEditorAlphaValueEditor にしましょう。役割を考えると "initial" と特化した名前を付ける必要はありません。
  • 可能ならいっそ LabelledSliderEditor みたいな設定可能なものにして汎用性を高めましょう。et.text(options) とかと同じパターンです。
{
  key: 'alpha',
  caption: 'Alpha for new labels',
  editor: laballedSlider({
    min: 0,
    max: 1,
    step: 0.1,
    label: value => `${Math.round(value * 100)}%`
  })
}

@ttakenaga ttakenaga marked this pull request as ready for review June 15, 2022 09:04
@smikitky
Copy link
Copy Markdown
Member

  • LabeledSlider を外部モジュールに切り分け、alpha みたいな文字が残っていたのを消し、型定義を整理しました。
  • Flexbox のアイテム間でスペースを空ける時は gap を使ってください。Flexbox, CSS Grid, 段組のいずれでも使えます。&nbsp; は基本的には最後の手段。

@smikitky smikitky enabled auto-merge June 16, 2022 02:28
@smikitky smikitky merged commit 9bffc26 into master Jun 16, 2022
@smikitky smikitky deleted the initial-alpha branch June 16, 2022 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants