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

Add ParamSetID to runs list in /jobs #720

Merged
merged 5 commits into from
Jan 29, 2021
Merged

Conversation

morimorihoge
Copy link
Collaborator

@morimorihoge morimorihoge commented Dec 25, 2020

refs: #714

既存の app/datatables/parameter_sets_list_datatable.rb の内容をベースにParamSetID(jobs.parameter_set_id)を表示するようにした。
-> 2021/01/08: Tooltiopの表示内容をParameterSet IDからSimulator Nameとパラメータリストを表示するように修正

Screenshots

before

image

after

image

要確認事項

マウスhoverして表示されるtooltipのIDが、table側に表示されているIDと明らかに違うが、これは問題ないか?
#shortened_id の実装上こうなること、既存の simulators#index でも同様の仕様でParamSetIDが表示されていることから仕様としては既存のものに揃っているが、ぱっと見バグにも見えてしまうのでやや気になる。
-> これはUUIDを使っている関係で先頭数文字はほぼ全部同じIDになってしまうことによる仕様。現状利用者側からも問題にはなっていないとのことなので、この仕様を踏襲して良い
--> その後、Tooltipの表示内容がSimulator Nameとパラメータリストに変更されたので、この違和感はなくなった

@morimorihoge morimorihoge changed the title Add ParamSetID to runs list in /jobs #714 Add ParamSetID to runs list in /jobs Dec 25, 2020
@morimorihoge morimorihoge self-assigned this Dec 25, 2020
@yohm
Copy link
Member

yohm commented Dec 25, 2020

ありがとうございます。tooltipで表示する中身ですが、ParameterSetのIDではなく、Simulatorの名前とパラメータの値を表示するようにしていただけませんんか?
例えば、以下のような感じです。Simulatorの名前は ps.simulator.name パラメータの値は ps.v にHashとして格納されています。

Simulator: NS_model
L: 100
Vmax: 5
density: 0.3
p_d: 0.1
t_init: 100
t_measure: 100

注意点として、ps.v の値(つまりHashの値)に別のHashが含まれる場合があります。その場合はonelineのJSON形式で表示していただけるとみやすいかと思います。

@morimorihoge
Copy link
Collaborator Author

tooltipの内容について、承知いたしました。
こちら対応してみます!

@morimorihoge
Copy link
Collaborator Author

遅くなりまして申し訳ありません。こちら、対応済みのモノをpushいたしましたのでご確認下さい。

PR descriptionのスクリーンショットも更新済みになります。

@morimorihoge
Copy link
Collaborator Author

定例にてコメントを頂きました。

  • 表示内容はこれで問題ないが、:で横位置を揃えられたらうれしい

ということで、tableなのかDL/DTなのかやれそうな方法を検討してみます。

@morimorihoge
Copy link
Collaborator Author

本件、取り急ぎ現状報告です。やや難航してしまっており引き続き調査中です。

状況としては、Bootstrap Tooltipのドキュメントを見つつHTML挿入するだけだと、どうも一部のタグが消える挙動が起こっています。

  • 挿入できたタグ
    • h1
    • b
    • span
  • 消えたタグ
    • table
    • dl / dt / dd

image

タグによっては挿入できることを見ると、HTMLエンティティのエスケープ問題などではなく、JSレベルでフィルタなどがされていそうに思うのですが、Bootstrap TooltipのJSコードを見る限りでは普通にjQuery.html()を使って挿入しているだけのように見えます。

ただ、以下のJSFiddleなどではtableが挿入できているので、Bootstrap Tooltipのバージョンなども原因かもしれない、という所で引き続き調査中です。

http://jsfiddle.net/56arN/

@morimorihoge morimorihoge force-pushed the feature/param_set_id-in-jobs branch 2 times, most recently from 40469eb to 890c1f7 Compare January 22, 2021 01:33
Copy link
Collaborator Author

@morimorihoge morimorihoge left a comment

Choose a reason for hiding this comment

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

調査に時間かかってしまいましたが諸々対応版に更新しました。

str + "<dt>#{k}:</dt><dd> #{v}</dd>"
end
html = <<EOS
<dl class='dl-horizontal'>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bootstrap純正のdescription listを使っています。

https://getbootstrap.com/docs/3.4/css/#description

app/assets/javascripts/bootstrap.js Show resolved Hide resolved
@morimorihoge
Copy link
Collaborator Author

@yohm 調査時間かかってしまいましたが、無事表形式表示ができましたのでご確認下さい。

原因等についてはソースコメントに書いておきました 🙇‍♂️

image

@yohm
Copy link
Member

yohm commented Jan 22, 2021

ありがとうございます。細かいところで恐縮なのですが、tooltipの中身がだいぶ右に寄せられているのと、一番下に空白行があるのでその部分を調整していただけないでしょうか?

Screen Shot 2021-01-22 at 10 50 37

@morimorihoge
Copy link
Collaborator Author

承知しました。

今確認してみたところ、Bootstrapのdescription listのCSS問題(widthやmarginに固定値が指定されている)のようでした。
こちら、修正の際ですが、全.dl-horizontal要素に適用してしまって良いか、tooltip内の.dl-horizontalにのみ適用した方が良いかなど希望あればお願いいたします。

#他でdlタグを使っていなければ全体適用してしまってもよさそうですが

@morimorihoge
Copy link
Collaborator Author

定例メモ:tooltip内の.dl-horizontalの上書きで対応する

before: parameter set ID
after: simulator name & parameters
tooltipのtag whitelistを更新することで、tooltip内でdl / dt / ddタグが使えるように修正している。
refer: https://getbootstrap.com/docs/3.4/javascript/#js-sanitizer
In bootstrap 3, dl tag has fixed width and margin but table doesn't.
@morimorihoge
Copy link
Collaborator Author

morimorihoge commented Jan 29, 2021

@yohm
色々と試行錯誤してみたところ、Bootstrap 3のDLタグは横幅が動的に変わるような要素をあまり想定しておらず、単純にwidthサイズ指定されている値を上書きするだけだと以下のようにoverflow扱いになってしまう(自動で伸張しない)ことが分かりました。

image

※なお、伸張するようにしようとすると、今度はDD側がmargin-leftで位置調整されているため別の問題が発生してしまっていました。

というわけで、DLタグを使うのを諦め、TABLEタグで書き直したところ、見る限りいい感じになったように見えます。

image

TABLEのスタイルは .table.table-condensed (ちょっと詰め気味のTABLE)にしつつ、borderを外したものになっています。
こちら、スタイルの方こちらで良ければ内容確認をお願いします。

In bootstrap 3, dl tag has fixed width and margin but table doesn't.
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 8db9cca into develop Jan 29, 2021
@yohm yohm deleted the feature/param_set_id-in-jobs branch January 29, 2021 03:13
@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.

Jobs ページにおけるテーブルに ParameterSet へのリンクを追加 する
2 participants