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

ウェブサイト #5

Merged
merged 63 commits into from
Mar 26, 2020
Merged

ウェブサイト #5

merged 63 commits into from
Mar 26, 2020

Conversation

atrn0
Copy link
Member

@atrn0 atrn0 commented Mar 21, 2020

とりあえずトップページだけ

クソデカPRにしてしまった🤷‍♂️

#1

@atrn0 atrn0 requested a review from a team March 21, 2020 05:15
@atrn0 atrn0 self-assigned this Mar 21, 2020
Comment on lines 2 to 13
<div>
<footer class="footer-root">
<div class="links">
<p v-for="(link, idx) in links" :key="idx">
<a :href="link.href" :target="link.target">
{{ link.title }}
</a>
</p>
</div>
<div class="has-text-centered">©2020 CAMPHOR-</div>
</footer>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

これってdivで囲わなくても良くない?

<footer class="footer-root">
<div class="links">
<p v-for="(link, idx) in links" :key="idx">
<a :href="link.href" :target="link.target">
Copy link
Member

Choose a reason for hiding this comment

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

rel="noopener noreferrer" を付けた方がよさそう

@@ -0,0 +1,66 @@
<template>
<component
:is="isInternalLink(path) ? 'nuxt-link' : 'a'"
Copy link
Member

Choose a reason for hiding this comment

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

これの書き方良いね 👍

@atrn0
Copy link
Member Author

atrn0 commented Mar 26, 2020

@p1ass 修正しました〜

Copy link
Member

@p1ass p1ass 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
Member

@honai honai left a comment

Choose a reason for hiding this comment

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

見てみました

}

.join {
> .show-schedule {
Copy link
Member

Choose a reason for hiding this comment

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

要らなそう

<template>
<div class="about-root">
<div class="container">
<div id="join" v-in-viewport class="section join">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div id="join" v-in-viewport class="section join">
<section id="join" v-in-viewport class="section join">

見出しがあるまとまりなので

transform: translateY(0px);
}
</style>
<script>
Copy link
Member

Choose a reason for hiding this comment

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

このファイルだけ <style> が先にあるので、統一したほうが良いかも

<div class="columns is-vcentered">
<div class="column is-two-fifths">
<img
src="https://github.com/camphor-/lab/raw/master/assets/connect-idea.png"
Copy link
Member

Choose a reason for hiding this comment

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

assetsにコピーして使ったほうがいいかも

</div>
<div class="column">
<img
src="https://github.com/camphor-/lab/raw/master/assets/available-resources.png"
Copy link
Member

Choose a reason for hiding this comment

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

ここもassetsにコピーしたらいいのでは

<div class="hero-background"></div>
<div class="lab-hero">
<div class="columns is-variable is-5 is-desktop">
<div v-in-viewport class="lab-title column has-text-white">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div v-in-viewport class="lab-title column has-text-white">
<section v-in-viewport class="lab-title column has-text-white">

見出しをもつまとまりなので

{{ link.title }}
</a>
</p>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

div > p > a でリストを作るより ul > li > a で (CSSリセットは必要になるが) したほうがセマンティックWebとしては良いかも

export default {
data() {
return {
links
Copy link
Member

Choose a reason for hiding this comment

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

変更できる値ではなく定数なので、 computed として返すほうが自然…? (ほんまか?)

</a>
</p>
</div>
<div class="has-text-centered">©2020 CAMPHOR-</div>
Copy link
Member

@honai honai Mar 26, 2020

Choose a reason for hiding this comment

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

Suggested change
<div class="has-text-centered">©2020 CAMPHOR-</div>
<div class="has-text-centered">&copy;2020 CAMPHOR-</div>

のほうが自分は好きです(ほぼ好みです)

>
{{ item.title }}
</a>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

ここも ul > li > a でリストにするのが自然かなと思いました

@atrn0
Copy link
Member Author

atrn0 commented Mar 26, 2020

@honai せまんちっくうぇぶにしました!

Copy link
Member

@honai honai left a comment

Choose a reason for hiding this comment

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

お疲れさまです :LGTM: 👍

@atrn0 atrn0 merged commit 32ed92c into master Mar 26, 2020
@atrn0 atrn0 deleted the website branch March 26, 2020 16:08
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.

3 participants