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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web/navbar hide #41

Merged
merged 18 commits into from Aug 13, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Aug 9, 2018

Closes #35

馃懛 Changes

  • Reworked navbar styles based on #39
  • Reworked navbar logic
  • Added animation for scroll
  • Updated demo

navbardemo-2

馃挱 Notes

Targeted at #39 for now for a clearer diff, do not merge! Will point to master once that is in.

馃敠 Testing Instructions

make web go to /ui_demo

To Do

  • Update tests 馃槩
@bobheadxi bobheadxi requested review from chamkank and mingyokim Aug 9, 2018
@bobheadxi bobheadxi added this to In progress in nwHacks 2019 via automation Aug 9, 2018
@bobheadxi bobheadxi moved this from In progress to Needs review in nwHacks 2019 Aug 9, 2018
@bobheadxi bobheadxi requested a review from sherryyx Aug 9, 2018
bobheadxi added 4 commits Aug 9, 2018
@bobheadxi bobheadxi changed the base branch from web/flexbox-utils to master Aug 9, 2018
Copy link

sherryyx left a comment

The transition from the background-fill state to the transparent background state looks rlly good!

After looking at it some more, I think we should just make the background fill 100% opaque. The transparency doesn't really add anything & makes it kind of distracting to read the nav bar. I've updated it in the figma document just now.

Other than that, it looks good to go 馃挴

Thanks!

@bobheadxi bobheadxi requested review from chamkank and mingyokim and removed request for chamkank and mingyokim Aug 11, 2018
鈥eb/navbar-hide
Copy link
Contributor

mingyokim left a comment

found a bug, will review the rest later

<nav>
<div>
<div>{getLogo()}</div>
class Navbar extends React.Component {

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 11, 2018

Contributor

I believe you need to add a componentShouldUpdate lifecycle method to update the state when new props are passed into this component.

Right now the component is not updating based on incoming props. Reproduction steps:

  • Make sure you're logged out
  • Go to /
  • Refresh and it should have logo, links, and login button
  • Click "login" button
  • The navbar still has logo, links, and login button
  • Refresh
  • The navbar is updated to just have logo

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 11, 2018

Author Contributor

Can't repro anymore after f66d3b0 - let me know if it works when you get the chance!

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

Yeah I can't reproduce it anymore. But wouldn't it be better to have displayType and buttonType in the state? The only reason why this component re-renders properly is because its container is re-rendering itself. If for some reason the container does not re-render itself, but the props change, then this component wouldn't re-render.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

Hm, I鈥檒l have a closer look at this tonight

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

@mingyokim sorry, I'm not sure I understand - doesn't a prop change force the container to re-render? From what I understand, displayType and buttonType are only manipulated from here, and the navbar itself does not change them:

https://github.com/nwhacks/nwhacks2019/blob/master/web/containers/navbar/index.js#L57

It also seems like setting props that a component doesn't manipulate into state is somewhat bad practice (https://stackoverflow.com/a/47341539), but there also seems to be some other APIs for doing this kind of stuff (https://stackoverflow.com/a/49839457, https://stackoverflow.com/a/32414771)... though to be honest this seems to be kind of murky territory, since from what I understand, this type of stuff should just be kept in props (https://stackoverflow.com/a/40330195). I could be missing something though...

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 13, 2018

Author Contributor

Officially, react only distinguishes between class and function (potentially stateful and stateless respectively), and:

The above two components are equivalent from React鈥檚 point of view.

https://reactjs.org/docs/components-and-props.html#functional-and-class-components

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 13, 2018

Contributor

so props do cause a render on stateful components? oops i was mistaken, it's all good then

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 13, 2018

Author Contributor

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:

Why you don't render props directly?

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 13, 2018

Contributor

An update can be caused by changes to props or state.

https://reactjs.org/docs/react-component.html

Okay, so clearly change in props cause a re-render, reactjs says so. My bad lol

If this is the case, definitely use props rather than state because the component is not modifying the buttonType or displayType, they're just displaying it.

Approving

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 13, 2018

Contributor

I'll just do one last sanity check

@bobheadxi bobheadxi force-pushed the web/navbar-hide branch 4 times, most recently from a13c8e1 to ca0bf1a Aug 11, 2018
@bobheadxi bobheadxi force-pushed the web/navbar-hide branch from ca0bf1a to 34b6748 Aug 11, 2018
Copy link
Contributor

mingyokim left a comment

minor comments - looks good!

@@ -1,43 +1,21 @@
nav
display: flex
z-index: 500

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

curious, why is this needed?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

This keeps the navbar above all other elements, so long as we avoid using z-index elsewhere

img
margin-top: 3px

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

spaces should always be multiples of 4 - can you change this to 4px? If the design said 3px that should be an error in design. @sherryyx can you confirm?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

This is a hack because the logo looked slightly off :(

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

4px looks fine too though - I'll make this change

<nav>
<div>
<div>{getLogo()}</div>
class Navbar extends React.Component {

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

Yeah I can't reproduce it anymore. But wouldn't it be better to have displayType and buttonType in the state? The only reason why this component re-renders properly is because its container is re-rendering itself. If for some reason the container does not re-render itself, but the props change, then this component wouldn't re-render.


// Calculate position
const offset = window.pageYOffset || document.documentElement.scrollTop;
const atTop = offset < 96;

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

Can we store 96 into some kind of variable?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

Sure - 96 is in the CSS as navbar height, but I鈥檒l replicate it here

const atTop = offset < 96;

// Calculate transparency
const transparent = offset < 256;

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

same here

const { hidden, transparent } = this.state;
const { displayType, buttonType } = this.props;
const button = getButton(buttonType);
const links = [

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

cleaann but can we name this something like linksAndButton? it doesn't just have links

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Aug 12, 2018

Author Contributor

True!


// repeating horizontal elements
&-horizontal
&-divs

This comment has been minimized.

Copy link
@mingyokim

mingyokim Aug 12, 2018

Contributor

nice

@bobheadxi bobheadxi requested a review from mingyokim Aug 12, 2018
nwHacks 2019 automation moved this from 馃攳 Pending Review to 馃弳 Approved Aug 13, 2018
Copy link
Contributor

mingyokim left a comment

Looks good, and thanks for correcting my misunderstanding on component rendering!

@bobheadxi bobheadxi merged commit 9f3835f into master Aug 13, 2018
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
nwHacks 2019 automation moved this from 馃弳 Approved to 馃梼 Closed Aug 13, 2018
@bobheadxi bobheadxi deleted the web/navbar-hide branch Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
nwHacks 2019
  
馃梼 Closed
3 participants
You can鈥檛 perform that action at this time.