Web/navbar hide #41
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React from 'react'; | ||
|
|
||
| const Main = () => (<div className="pad-nav">HOME PAGE</div>); | ||
|
|
||
| export default Main; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| div#demo | ||
| width: 50% | ||
| margin-left: 25% | ||
| height: 90000000px |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,43 +1,21 @@ | ||
| nav | ||
| z-index: 500 | ||
| background: $white | ||
| height: 64px | ||
| position: fixed | ||
| transition: background $transition-time-long, shadow $transition-time-long, opacity $transition-time | ||
|
|
||
| a | ||
| text-decoration: none | ||
| font-family: Apercu Pro | ||
| font-size: 18px | ||
|
|
||
| img | ||
| margin-top: 3px | ||
|
There was a problem hiding this comment. spaces should always be multiples of 4 - can you change this to There was a problem hiding this comment. This is a hack because the logo looked slightly off :( There was a problem hiding this comment. 4px looks fine too though - I'll make this change |
||
| width: 32px | ||
|
|
||
| &.transparent | ||
| background: transparent | ||
|
|
||
| &.hide | ||
| opacity: 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,11 @@ | ||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { Link } from 'react-router-dom'; | ||
| import { DISPLAY_TYPE } from '../../containers/navbar/DisplayTypes'; | ||
| import { BUTTON_TYPE } from '../../containers/navbar/ButtonTypes'; | ||
| import { SecondaryButton } from '../input/buttons'; | ||
|
|
||
| import logo from '../../assets/logo.svg'; | ||
|
|
||
| const getButton = (buttonType) => { | ||
| switch (buttonType) { | ||
|
|
@@ -23,43 +20,95 @@ const getButton = (buttonType) => { | |
| } | ||
| }; | ||
|
|
||
| class Navbar extends React.Component { | ||
|
There was a problem hiding this comment. I believe you need to add a Right now the component is not updating based on incoming props. Reproduction steps:
There was a problem hiding this comment. Can't repro anymore after f66d3b0 - let me know if it works when you get the chance! There was a problem hiding this comment. Yeah I can't reproduce it anymore. But wouldn't it be better to have There was a problem hiding this comment. Hm, I’ll have a closer look at this tonight There was a problem hiding this comment. @mingyokim sorry, I'm not sure I understand - doesn't a prop change force the container to re-render? From what I understand, https://github.com/nwhacks/nwhacks2019/blob/master/web/containers/navbar/index.js#L57 It also seems like setting There was a problem hiding this comment. Officially, react only distinguishes between class and function (potentially stateful and stateless respectively), and:
https://reactjs.org/docs/components-and-props.html#functional-and-class-components There was a problem hiding this comment. so props do cause a render on stateful components? oops i was mistaken, it's all good then There was a problem hiding this comment. Similar question: https://www.reddit.com/r/reactjs/comments/7tn79g/state_and_componentwillreceiveprops/ class Foo extends Component {
state = { value: this.props.value }
// If the props change, update state
componentWillReceiveProps (nextProps) {
if (nextProps.value !== this.props.value) {
this.setState({ value: nextProps.value })
}
}
render () {
// render based on state.value
}
}Top response:
There was a problem hiding this comment.
https://reactjs.org/docs/react-component.html Okay, so clearly change in props cause a re-render, If this is the case, definitely use props rather than state because the component is not modifying the Approving There was a problem hiding this comment. I'll just do one last sanity check |
||
| constructor(props) { | ||
| super(props); | ||
| const { displayType, buttonType } = props; | ||
| this.state = { | ||
| displayType, | ||
| buttonType, | ||
| hidden: false, | ||
| transparent: true, | ||
| lastY: 0, | ||
| }; | ||
| } | ||
|
|
||
| componentDidMount() { | ||
| window.addEventListener('scroll', this.handleScroll, { passive: true }); | ||
| } | ||
|
|
||
| componentWillUnmount() { | ||
| window.removeEventListener('scroll', this.handleScroll); | ||
| } | ||
|
|
||
| handleScroll = () => { | ||
| // Calculate scroll direction | ||
| const { lastY } = this.state; | ||
| const scrollingDown = (window.scrollY - lastY) >= 0; | ||
|
|
||
| // Calculate position | ||
| const offset = window.pageYOffset || document.documentElement.scrollTop; | ||
| const atTop = offset < 96; | ||
|
There was a problem hiding this comment. Can we store There was a problem hiding this comment. Sure - 96 is in the CSS as navbar height, but I’ll replicate it here |
||
|
|
||
| // Calculate transparency | ||
| const transparent = scrollingDown ? offset < 256 : atTop; | ||
|
|
||
| // Update state | ||
| this.setState({ | ||
| transparent, | ||
| hidden: (!atTop && scrollingDown), | ||
| lastY: window.scrollY, | ||
| }); | ||
| } | ||
|
|
||
| render() { | ||
| const { displayType, buttonType, hidden, transparent } = this.state; | ||
| const button = getButton(buttonType); | ||
| const links = [ | ||
|
There was a problem hiding this comment. cleaann but can we name this something like |
||
| <Link to="/"><b>About</b></Link>, | ||
| <Link to="/"><b>Stories</b></Link>, | ||
| <Link to="/"><b>FAQ</b></Link>, | ||
| <Link to="/"><b>Sponsors</b></Link>, | ||
| <Link to="/"><b>2018</b></Link>, | ||
| button, | ||
| ]; | ||
|
|
||
| let linksDiv; | ||
| let key = 0; | ||
| switch (displayType) { | ||
| case DISPLAY_TYPE.ONLY_LOGO: | ||
| break; | ||
| case DISPLAY_TYPE.LOGO_AND_BUTTON: | ||
| linksDiv = <div className="flex ai-center">{button}</div>; | ||
| break; | ||
| default: | ||
| linksDiv = links.map(l => ( | ||
| <div | ||
| key={key += 1} | ||
| className="flex ai-center margin-sides-large"> | ||
| {l} | ||
| </div> | ||
| )); | ||
| } | ||
|
|
||
| return ( | ||
| <nav className={`fill-width flex ${hidden ? 'hide' : ''} ${transparent ? 'transparent' : 'shadow'}`}> | ||
| <div className="flex ai-center jc-start margin-sides"> | ||
| <div className="flex ai-center"> | ||
| <Link to="/"><img alt="nwHacks" src={logo} /></Link> | ||
| </div> | ||
| </div> | ||
| <div className="flex jc-end margin-horizontal-divs fill-width"> | ||
| {linksDiv} | ||
| </div> | ||
| </nav> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Navbar.propTypes = { | ||
| displayType: PropTypes.symbol, | ||
| buttonType: PropTypes.symbol, | ||
| }; | ||
|
|
||
| export default Navbar; | ||
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.
curious, why is this needed?
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.
This keeps the navbar above all other elements, so long as we avoid using z-index elsewhere