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
Imprv/fix subnavigation #1839
Imprv/fix subnavigation #1839
Conversation
rrih
commented
Apr 2, 2020
import ReducedPageCreator from './ReducedPageCreator'; | ||
import ReducedRevisionAuthor from './ReducedRevisionAuthor'; | ||
|
||
const GrowiCompactSubNavigation = (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これはコンポーネント新設ではなく、GrowiSubNavigation をリサイクルして、コンパクトモードを実装して
|
||
</div> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isCompactMode で完全に return をするのではなく、部分的に部品を入れ替えるべきだと思う。基本レイアウトは変わらないはずなので。
</div> | ||
</div> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これらも同様、isCompactMode で完全に return をするのではなく、部分的に部品を入れ替えるべきだと思う。
@@ -20,5 +20,22 @@ | |||
} | |||
} | |||
} | |||
.grw-compact-subnavbar { | |||
background-color: rgba(250, 250, 250, 0.9); | |||
box-shadow: 0 0 2px #999; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
色はテーマ側で決める
.grw-compact-subnavbar { | ||
background-color: rgba(250, 250, 250, 0.9); | ||
box-shadow: 0 0 2px #999; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 色の変数は各テーマで用意し、このファイルで apply する
- または、apply-colors-light/dark で、色を固定して apply する
- または、色の変数は各テーマで用意し、apply-colors-light/dark で apply する
@@ -20,6 +20,8 @@ const GrowiSubNavigation = (props) => { | |||
const { | |||
path, createdAt, creator, updatedAt, revisionAuthor, | |||
} = pageContainer.state; | |||
const isCompactMode = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
概ね良いと思う。
あとここを props 見るようにするだけかな?
const { revisionAuthor, updatedAt } = props; | ||
const isCompactMode = true; | ||
const { revisionAuthor, updatedAt, isCompactMode } = props; | ||
// const isCompactMode = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この行は削除
@@ -26,6 +25,7 @@ PageCreator.propTypes = { | |||
|
|||
creator: PropTypes.object.isRequired, | |||
createdAt: PropTypes.string.isRequired, | |||
isCompactMode: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required にはしなくていいと思う。代わりにデフォルト値を用意する。
@@ -26,6 +26,7 @@ RevisionAuthor.propTypes = { | |||
|
|||
revisionAuthor: PropTypes.object.isRequired, | |||
updatedAt: PropTypes.string.isRequired, | |||
isCompactMode: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required にはしなくていいと思う。代わりにデフォルト値を用意する。
@@ -64,6 +65,9 @@ export default class PageContainer extends Container { | |||
this.save = this.save.bind(this); | |||
this.addWebSocketEventHandlers = this.addWebSocketEventHandlers.bind(this); | |||
this.addWebSocketEventHandlers(); | |||
|
|||
const scrollAmountForFixed = 122; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この値は 9 行目付近に const として用意する(static 変数っぽいイメージ)
@@ -64,6 +65,9 @@ export default class PageContainer extends Container { | |||
this.save = this.save.bind(this); | |||
this.addWebSocketEventHandlers = this.addWebSocketEventHandlers.bind(this); | |||
this.addWebSocketEventHandlers(); | |||
|
|||
const scrollAmountForFixed = 122; | |||
window.addEventListener('scroll', () => { this.setState({ isCompactMode: window.pageYOffset > scrollAmountForFixed }) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.npmjs.com/package/throttle-debounce
の throttle を使ってください。
{revisionAuthor != null && <li className="mt-1"><RevisionAuthor revisionAuthor={revisionAuthor} updatedAt={updatedAt} /></li>} | ||
{creator != null && <li><PageCreator creator={creator} createdAt={createdAt} isCompactMode={isCompactMode} /></li>} | ||
{revisionAuthor != null | ||
&& <li className="mt-1"><RevisionAuthor revisionAuthor={revisionAuthor} updatedAt={updatedAt} isCompactMode={isCompactMode} /></li>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここインデントを工夫してもう少し読みやすく
# Conflicts: # src/client/js/components/Navbar/GrowiSubNavigation.jsx