Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ParameterSet table UI #729

Merged
merged 4 commits into from
Feb 12, 2021
Merged

Conversation

yusiro-bps
Copy link
Collaborator

@yusiro-bps yusiro-bps commented Feb 4, 2021

refs: #716

対応内容

  • パラメータ列を並び替えられるようにする
  • スクロールしてもパラメータの名前が表示されるようにする
  • 画面上部にも次のページ、前のページに移動するボタンをつける
  • 値の大小の順位に応じてセルの色をグラデーションする

Untitled

@yusiro-bps yusiro-bps marked this pull request as draft February 4, 2021 07:54
@yusiro-bps yusiro-bps force-pushed the feature/impove_parameter_sets_table branch 5 times, most recently from 99a948c to 2899433 Compare February 9, 2021 07:24
@@ -1,4 +1,5 @@
class ParameterSetsListDatatable
HUE_MAX = 215
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HSLでセルの色を指定するようにしました。
値が大きいものから小さいものへ、赤(0)から青(215)に変化するようにしています(青が濃すぎると数値が見辛くなるので適当に215を最大値として設定しています)。

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます。セルへの色付けイメージ通りです。
色をもう少し全般的に透明度をつけて薄い色にしていただけますか?背景がうっすらと色付けされているくらいで良いかと思います。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

コメントありがとうございます。hslaの透明度を0.3で設定してみました。

image

@@ -40,28 +41,28 @@ def sort_by
end

def data
data = []
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

列の並び替え後にサーバ側とフロント側で並び順の整合性を取るため、dataの形を以下のような形に変えています。

"data": [
    {
      "Checkbox": "...",
      "Progress": "...",
      "ParamsSetID": "...",
      "Updated_at": "...",

    },
    {
      "Checkbox": "...",
      "Progress": "...",
      "ParamsSetID": "...",
      "Updated_at": "...",

    },
  ]

Copy link
Member

Choose a reason for hiding this comment

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

Simulatorが持つパラメータの定義として "Checkbox", "Progress"などの文字をとることも原理的にはできるので、ここを重複しないようにできないでしょうか?("Progress" はパラメータの定義としてありうるかもしれないので)
パラメータの定義は letter, number, underscore しかとることができないので、"Progress" を "-Progress" などのようにすれば一意性を保証できます。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ご指摘ありがとうございます。固定カラムは"-"を先頭につけるように修正しました。

tmp << @view.raw('<span class="ps_updated_at">'[email protected]_to_now_in_words(ps.updated_at)+'</span>')
tmp.store('Progress', @view.raw(progress))
tmp.store('ParamSetID', @view.link_to( @view.shortened_id_monospaced(ps.id), @view.parameter_set_path(ps), data: {toggle: 'tooltip', placement: 'bottom', html: true, 'original-title': _tooltip_title(ps)} ))
tmp.store('Updated_at', @view.raw('<span class="ps_updated_at">'[email protected]_to_now_in_words(ps.updated_at)+'</span>'))
@param_keys.each do |key|
if @base_ps
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

List of Similar Parameter Sets のテーブルでは、数値に色がつくようなので、セルに色はつけていませんが、必要であればお知らせください。

Copy link
Member

Choose a reason for hiding this comment

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

おっしゃる通り、必要ないと思います。現状のままでお願いします

@yusiro-bps yusiro-bps force-pushed the feature/impove_parameter_sets_table branch from 2899433 to 592a4de Compare February 9, 2021 09:57
@yusiro-bps yusiro-bps force-pushed the feature/impove_parameter_sets_table branch from 592a4de to 53c229b Compare February 10, 2021 01:18
@@ -232,3 +232,10 @@ input[type="checkbox"] {
.alert {
padding: 10px;
}

table.dataTable.fixed_thead thead th {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://datatables.net/extensions/fixedheader という拡張機能があるのですが、それを使うと、チェックボックス全選択が効かなくなったり、別タブを開いた際にヘッダーが残るようだったので、CSSで固定しています。

url: $(selector).data('source'),
data: function (d) {
const data = JSON.parse(localStorage.getItem(`DataTables_${$(selector)[0].id}_`+window.location.pathname));
d.sort_column = data ? data['order'][0][0] : null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

列並び替えのあと、列ソートした際にサーバ側でフロント側で指定した列を識別できるようにソート対象列のindexを新しくパラメータとして追加しています。

@yusiro-bps yusiro-bps requested a review from yohm February 10, 2021 01:57
@yusiro-bps yusiro-bps marked this pull request as ready for review February 10, 2021 01:57
Copy link
Member

@yohm yohm left a comment

Choose a reason for hiding this comment

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

試しに使ってみましたが、おかげさまでとても使いやすくなっています!1点だけ非常に細かい点ですがコメントしました。

@@ -40,28 +41,28 @@ def sort_by
end

def data
data = []
Copy link
Member

Choose a reason for hiding this comment

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

Simulatorが持つパラメータの定義として "Checkbox", "Progress"などの文字をとることも原理的にはできるので、ここを重複しないようにできないでしょうか?("Progress" はパラメータの定義としてありうるかもしれないので)
パラメータの定義は letter, number, underscore しかとることができないので、"Progress" を "-Progress" などのようにすれば一意性を保証できます。

@yusiro-bps yusiro-bps requested a review from yohm February 12, 2021 01:03
Copy link
Member

@yohm yohm left a comment

Choose a reason for hiding this comment

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

ありがとうございます!

@yohm yohm merged commit 215d2b7 into develop Feb 12, 2021
@yohm yohm deleted the feature/impove_parameter_sets_table branch February 12, 2021 01:56
@yohm yohm added this to the Improvement-202103 milestone Apr 23, 2021
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