Skip to content

Shunsuke#25

Open
nihei-shunsuke wants to merge 21 commits intomainfrom
shunsuke
Open

Shunsuke#25
nihei-shunsuke wants to merge 21 commits intomainfrom
shunsuke

Conversation

@nihei-shunsuke
Copy link
Copy Markdown
Collaborator

スクリーンショット (132)
Cardコンポーネントを作成し、文字のずれを修正
App.cssでCardコンポーネントを中央に揃えた(本当は縦二列にしたかった)

@nihei-shunsuke nihei-shunsuke added the review wating for review label Jan 29, 2021
@tomu28
Copy link
Copy Markdown
Collaborator

tomu28 commented Jan 31, 2021

App.cssでCardコンポーネントを中央に揃えた(本当は縦二列にしたかった)

ヒント

スクリーンショット 2021-01-31 18 46 39

@nihei-shunsuke
Copy link
Copy Markdown
Collaborator Author

nihei-shunsuke commented Jan 31, 2021

before after
before after

Copy link
Copy Markdown
Collaborator

@tomu28 tomu28 left a comment

Choose a reason for hiding this comment

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

気になった点をレビューしました👍

PR名はそのPRで行った内容を簡易的に示すものに変更しておこう!

Comment thread src/App.css Outdated
text-align: center;
}
.App{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nits]
不要な改行は削除しておこう👍

Comment thread src/App.js
Learn React
</a>
</header>
<Card />
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nice]
ナイスリファクタ

Comment thread src/App.js
import './App.css';

import Card from './component/Card';
function App() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nits]
import文の後に1行改行があると見やすいね

Comment thread src/component/Card.js
import Sukiya from './img/sukiya.png';
import Wego from './img/wego.png';
import Earth from './img/earth.jpg';
import GU from './img/GU.png';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nits]
ここも1行改行あると見やすいね

Comment thread src/component/Card.js
{img: Earth, alt:'earth', shop:'アースミュージック&エコロジー 金沢フォーラス店', distance:6.7},
{img: GU, alt:'GU', shop:'GU イオンタウン金沢示野店', distance:8.3}
];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change

[Nits]
意図がなければ削除しても🙆‍♂️

Comment thread src/component/Card.js Outdated
class Card extends Component {
render() {
return (
<div>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Q]
このdiv要素は必要かな?

Comment thread src/component/card.css Outdated
flex-wrap: wrap;
justify-content: space-between;
padding: 2.8125rem 0 0 4.375rem;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Must]
全体的にインデントが1レベルずれているので直そう👍

@nihei-shunsuke nihei-shunsuke requested a review from tomu28 January 31, 2021 10:31
Comment thread src/App.css Outdated
@@ -1,8 +1,7 @@
.App{

/* .App{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Imo]
今後使用するなど意図がなければ削除していいよ👍

Comment thread src/component/Card.js Outdated
class Card extends Component {
render() {
return (
<div className="container">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Q]
ここインデントあってるかな?

Comment thread src/component/card.css
font-size: 1.875rem;
color: #696969;
}
.container {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nits]
ここも改行入れていいね

Comment thread src/component/Card.js Outdated
</div>
</a>
</div>
))}
Copy link
Copy Markdown
Collaborator

@tomu28 tomu28 Jan 31, 2021

Choose a reason for hiding this comment

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

[Nits]
ここのインデントずれているね😅
ESLintを導入しよう👍

@tsuru-kazu
Copy link
Copy Markdown
Collaborator

tsuru-kazu commented Feb 2, 2021

@nihei-shunsuke
fetchしてHEADとFETCH_HEADの差分を見てたんだけど、sampleディレクトリにあるtempUIっていうのがどこかのタイミングで消えてるっぽいんだけど、どこで消してる?
コミットメッセージを見た感じだと、そのような変更は無かったから気になった

tsuru-kazu
tsuru-kazu previously approved these changes Feb 2, 2021
@tsuru-kazu tsuru-kazu self-requested a review February 2, 2021 08:28
@tsuru-kazu tsuru-kazu dismissed their stale review February 2, 2021 08:30

ミスった

@maresuke3
Copy link
Copy Markdown
Owner

maresuke3 commented Feb 2, 2021

sampleディレクトリにあるtempUIっていうのがどこかのタイミングで消えてるっぽいんだけど、どこで消してる?

tempUIディレクトリをマージして以降ニケがリモートマスターのブランチを一度も取り込んだ形跡がNetworkグラフに出ていなかったので恐らくリモートのマスターを取り込めば出てくると思います。

@maresuke3
Copy link
Copy Markdown
Owner

@nihei-shunsuke
ブランチがコンフリクトを起こしているので直してください。

Copy link
Copy Markdown
Collaborator

@tsuru-kazu tsuru-kazu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Collaborator

@tomu28 tomu28 left a comment

Choose a reason for hiding this comment

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

レビュー内容修正出来ていてLGTM

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review wating for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants