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

キャプションの上または下を設定するcaption_positionの実装 #1361

Merged
merged 10 commits into from
Jun 24, 2020

Conversation

kmuto
Copy link
Owner

@kmuto kmuto commented Aug 12, 2019

#1320 のcaption_positionの実装です。

refactoring-headerブランチから継承しているので、前の実装よりはかなり読みやすくはなかったかと思います。

副作用として、topbuilderのimageはこれまで上キャプションでしたが、ほかのビルダ合わせで下キャプションに変わります。

@kmuto kmuto requested a review from takahashim August 12, 2019 13:59
@kmuto
Copy link
Owner Author

kmuto commented Aug 25, 2019

  • 表現側のほうなので、ビルダで上とか下とか判断になるのは妥当
  • headerという名前が実は妥当なのか問題。begin/header/body/footer/end +caption?

@kmuto kmuto mentioned this pull request Sep 16, 2019
@kmuto kmuto mentioned this pull request Oct 29, 2019
28 tasks
@kmuto kmuto changed the base branch from refactoring-header to master December 17, 2019 02:40
@kmuto
Copy link
Owner Author

kmuto commented Dec 17, 2019

@takahashim
ひととおり整理してみました。

@kmuto kmuto mentioned this pull request Jun 12, 2020
@kmuto
Copy link
Owner Author

kmuto commented Jun 21, 2020

方針と基本実装はこれ以上あまりどうにもならなそうに思うので、テストケースを作ります(いくつか漏れがあったので修正中)。

@kmuto
Copy link
Owner Author

kmuto commented Jun 21, 2020

テストケースを作成し、HTML,LATEX,TEXT,IDGXMLについて期待の動作をするようにしました。

@takahashim
Copy link
Collaborator

こちらは機能的にはマージして問題ないかと思いますが、メソッド名は変えたほうが良いかもです。
top?('list') とかだとリスト自体の位置に見えて、キャプションのことだとは思わなさそう…。

素直に実装するとcaption_position('list') == :top or :bottomみたいになりそうですが、使い勝手はたいして良くなさそうなので、 caption_top?('list') くらいがよいでしょうか。

@kmuto
Copy link
Owner Author

kmuto commented Jun 23, 2020

おぉ、ご提案ありがとうございます! topは微妙だなーと思っていたのでcaption_top?にしておきます。

@kmuto kmuto merged commit b433dad into master Jun 24, 2020
@kmuto kmuto deleted the caption-position branch June 24, 2020 00:10
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