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.

Sign up for GitHub

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

Jump to bottom

[Add] gatsby-images導入 #64

Merged
merged 8 commits into from
Apr 5, 2020
Merged

[Add] gatsby-images導入 #64

merged 8 commits into from
Apr 5, 2020

Conversation

yuki-js
Copy link
Contributor

@yuki-js yuki-js commented Apr 1, 2020 •
edited
Loading

修正するIssue

#18

変更点

@yuki-js
Copy link
Contributor Author

yuki-js commented Apr 2, 2020 •
edited
Loading

CSSの一部プロパティを消したりしてますが、表示は問題ないと思います。
デザインを元に戻したほうがいいならissue立ててください。

@yuki-js yuki-js marked this pull request as ready for review April 2, 2020 02:50
@yuki-js yuki-js changed the title [WIP] Introduce gatsby-images etc. [Add] gatsby-images導入 Apr 2, 2020
AsagaKosho
AsagaKosho requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@AsagaKosho AsagaKosho left a comment

Choose a reason for hiding this comment

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

@yuki-js
いくつかlintのwarningが出ていますので対応お願いいたします

@nandenjin
その他の点について確認お願いします

@yuki-js
Copy link
Contributor Author

yuki-js commented Apr 2, 2020

Fixed

nandenjin
nandenjin requested changes Apr 2, 2020
View reviewed changes
src/components/BigImg.js Outdated Show resolved Hide resolved
src/components/Img.js Outdated Show resolved Hide resolved
src/components/OtherImg.js Outdated Show resolved Hide resolved
src/components/SmallPoster.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/templates/PostTemplate.js Outdated Show resolved Hide resolved
nandenjin
nandenjin requested changes Apr 2, 2020
View reviewed changes
Copy link
Collaborator

@nandenjin nandenjin left a comment

Choose a reason for hiding this comment

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

1回目のレビューと同様です！よろしくお願いします

@nandenjin nandenjin mentioned this pull request Apr 3, 2020
Closed
@AsagaKosho
Copy link
Member

@yuki-js
@nandenjin さんのレビュー、対応よろしくお願いします

@nandenjin nandenjin mentioned this pull request Apr 3, 2020
Merged
@AsagaKosho AsagaKosho self-requested a review April 3, 2020 09:00
AsagaKosho
AsagaKosho requested changes Apr 3, 2020
View reviewed changes
Copy link
Member

@AsagaKosho AsagaKosho left a comment

Choose a reason for hiding this comment

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

Conflictの修正お願いします

@nandenjin
Copy link
Collaborator

nandenjin commented Apr 3, 2020 •
edited by AsagaKosho
Loading

@yuki-js
こちら #64 (comment) のコンポーネントの命名について、名前から具体的な仕様が明らかでなく、またこのままサイト内でのBig/Smallの基準をこのコンポーネントの実装のみに委ねることはできないので、何かしらの対策が必要と思っています。

以下どちらかが取りうる解決策かなぁと思うのですが、他の案があればぜひお寄せください🙇‍♂️

  • なんらかの形で命名により具体的な要素を入れる
    • Ex: サイズについて名前に含める
    • Ex: ページ固有のコンポーネントとし、ページ名を含める（or ファイルを分けないで実装する）
  • Big/Smallなどの単語について、具体的な基準を明文化し、このコンポーネント以外でも再利用できるようにする。
    • スタイルシートのブレークポイントが似た概念だと思います。これを値として流用しても良いかもです🤔

もっといい案があればどんどんご意見いただきたいです。どうぞよろしくお願いします🙏

  • （上記コメントで稲田の提案した命名法、確かどこかで読んだものを参考にして使っているのですが、ソースが見つかりませんでした……すみません……）
  • （直接関わりはないですが、こういう時ってBigではなくLarge（-Medium-Small）っていう命名しませんかね？根拠となるものがあればスタイルシートでのブレークポイントの命名くらいしかないので、これもどなたか意見あれば欲しいです）

nandenjin
nandenjin reviewed Apr 3, 2020
View reviewed changes
Copy link
Collaborator

@nandenjin nandenjin left a comment

Choose a reason for hiding this comment

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

#64 (comment) 🙏

@nandenjin nandenjin added enhancement New feature or request help wanted Extra attention is needed labels Apr 3, 2020
@yuki-js
Copy link
Contributor Author

yuki-js commented Apr 3, 2020 •
edited
Loading

前者の方法を採用しFluid300とかFixed500と、クエリに基づいて命名するといいかと思いますが、どうでしょうか？賛同いただければ採用します。

@nandenjin
Copy link
Collaborator

@yuki-js ありがとうございます……！よろしくお願いします:pray:

AsagaKosho
AsagaKosho reviewed Apr 4, 2020
View reviewed changes
Copy link
Member

@AsagaKosho AsagaKosho left a comment

Choose a reason for hiding this comment

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

ありがとうございます！ 👍

@AsagaKosho AsagaKosho self-requested a review April 4, 2020 16:36
nandenjin
nandenjin approved these changes Apr 5, 2020
View reviewed changes
Copy link
Collaborator

@nandenjin nandenjin left a comment

Choose a reason for hiding this comment

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

LGTM

@nandenjin
Copy link
Collaborator

@yuki-js あと申し訳ないんですが今後のcommitではContribution Guideを再度確認の上、メッセージへのプレフィックスの付与を徹底願います……:pray:

@nandenjin nandenjin merged commit 6e6aef8 into tsukuba-shinkan:dev Apr 5, 2020
@nandenjin nandenjin mentioned this pull request Apr 5, 2020
Closed
@yuki-js yuki-js mentioned this pull request Apr 5, 2020
Closed
@nandenjin nandenjin mentioned this pull request Apr 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nandenjin nandenjin nandenjin approved these changes

@AsagaKosho AsagaKosho AsagaKosho approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yuki-js @AsagaKosho @nandenjin