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

Why isn't the breakpoints and currentBreakPoints logic within the Media component? #11

Open
joetidee opened this issue Aug 8, 2018 · 16 comments

Comments

@joetidee
Copy link
Contributor

joetidee commented Aug 8, 2018

Is there any reason why this package isn't written so that it can be used like this:

<Media breakpoint="mobile">
  ...
</Media>
<Media breakpoint="desktop">
  ...
</Media>

So that the children are conditionally rendered?

I'm a little confused by your render props example in the readme file, should it be:

import { Media } from 'react-breakpoints'

class Navigation extends React.Component {
  render() {
    return (
      <Media render={({ breakpoints, currentBreakpoint }) =>
          breakpoints[currentBreakpoint] > breakpoints.desktop ? (
            <DesktopNavigation />
          ) : (
            <TouchNavigation />
          )
        }
      />
    )
  }
}

export default Navigation

As surely the breakpoints are captured in the Media component using context and currentBreakpoint is calculated by Media?

@ehellman
Copy link
Owner

Hello @joetidee and thanks for opening an issue! :)

First question:

Yes, there's a few reasons why this is the case. Firstly, it would make it impossible to pass the breakpoints object and currentBreakpoint values down as actual props for the higher order component withBreakpoints, which is mainly used when you want to perform operations inside componentDidMount() or other lifecycle methods.

The other reason is that it is not only used to toggle components, but also props on components amongst other things and you would not be able to do the following:

<Media>
  {({ breakpoints, currentBreakpoint }) => (
    <div>
      <Select 
        autoComplete={breakpoints[currentBreakpoint] > breakpoints.tabletLandscape}
        useNativeSelect={breakpoints[currentBreakpoint] < breakpoints.tablet}
      />
    </div>
  )}
</Media>

The above example works equally well with the HOC btw.

Second question:

You are correct that the Media component is using context, however it is only the consumer and the calculation happens in ReactBreakpoints which wraps and feeds the values to the Provider. The Media component is actually the Context.Consumer component taken directly from the React.createContext() function, see here.

I want to make sure that you feel like you have gotten a proper answer to your second question, in case my explanation was bad, let me know and I'll come up with another example or create a small video explaining the affected code! :)

@ValentinH
Copy link
Contributor

ValentinH commented Aug 10, 2018

@joetidee about why not using the render property (as it is the case for context consumer), it's just that here this component is using the children instead of the render property. Most librairies do this as well as it's a bit easier to read.

@joetidee
Copy link
Contributor Author

joetidee commented Aug 12, 2018

Some really great responses - thank you. It's great that this package allows for flexibility in lots of use cases. I guess it would be nice to to have shortcut options that would keep my code tidy. I imagine that just needing to be able to not render components based solely on the viewport would be needed 95% of the time. So maybe introduce optionally importable Mount/UnMount component so that this can be done:

<Mount breakPoint="tablet">
  ...
</Mount>
<UnMount breakpoint="desktop">
  ...
</UnMount>

Now we would have the ability to import the correct components based on the complexity of the use case.

@joetidee joetidee changed the title Why aren't breakpoints and currentBreakPoints login in Media component? Why isn't the breakpoints and currentBreakPoints logic within the Media component? Aug 12, 2018
@ehellman
Copy link
Owner

@joetidee That is definitively a better solution, especially for the cases that you give an example to.

Added "Projects" to this repo and trying to add this issue there so that it's in the "todo" list :)

Imagining this:

ComponentName.propTypes = {
  // string matching one of the keys inside breakpoints object
  // not sure if it is possible to make dynamic like this with oneOf though
  breakpoint: PropTypes.oneOf(Object.keys(breakpoints)),
  // locks it to only show on the breakpoint which is supplied in props.breakpoint
  fixed: PropTypes.bool,
}

If anyone else has suggestions of what else to add here, feel free to chime in.

@joetidee
Copy link
Contributor Author

joetidee commented Aug 12, 2018

import { Consumer as Media } from 'react-breakpoints';

export const Mount = ({ breakpoint, children }) => {
   return (
      <Media>
         {
            ({ breakpoints, currentBreakpoint }) => 
               breakpoints[currentBreakpoint] >= breakpoints[breakpoint] ? children : null
         } 
      </Media>
   )
};

export const UnMount = ({ breakpoint }) => {
   return (
      <Media>
         {
            ({ breakpoints, currentBreakpoint }) => 
               breakpoints[currentBreakpoint] >= breakpoints[breakpoint] ? null : children
         } 
      </Media>
   )
};

@ehellman
Copy link
Owner

ehellman commented Aug 13, 2018

const MediaQuery = ({
  breakpoint,
  fixed,
  breakpoints,
  currentBreakpoint,
  children,
}) => 
  fixed
    ? breakpoints[currentBreakpoint] === breakpoints[breakpoint]
      ? children
      : null
    : breakpoints[currentBreakpoint] >= breakpoints[breakpoint]
      ? children
      : null

This would open up for the pattern you described in your first post, including fixed prop which can lock the component to the breakpoint you passed, showing it only if currentBreakpoint matches the value.

@ValentinH
Copy link
Contributor

This API would indeed be simpler to use, however we should still keep the render props/HoC api. Indeed, as explained in https://github.com/ReactTraining/react-media#usage:

If you use a regular React element as children (i.e. ) it will be rendered if the query matches. However, you may end up creating a bunch of elements that won't ever actually be rendered to the page (i.e. you'll do a lot of unnecessary createElements on each render).

So it's a bit worse performance wise.

@joetidee
Copy link
Contributor Author

Why do you feel that there is a need for the fixed prop?

@ehellman
Copy link
Owner

@joetidee To cover the scenario of when you want the children to only be visible on that single breakpoint, and not breakpoint and up.

@joetidee
Copy link
Contributor Author

joetidee commented Aug 22, 2018

Maybe we just need min/max breakpoint settings to cover every scenario. For example:

<Mount maxBreakpoint="mobile">
  // show on mobile only
</Mount>
<Mount minBreakpoint="mobile" maxBreakpoint="tablet">
  // show on tablet and mobile only
</Mount>
<Mount minBreakpoint="desktop">
  // Show on desktop (and above)
</Mount>

@joetidee
Copy link
Contributor Author

Are the contributors happy for me to take this on? Are we agreed on a way forward?

@ValentinH
Copy link
Contributor

For me this is fine, however I'm not a big fan of the <Mount /> component name... MediaQuery like @ehellman showed in his example. Even if it might be confusing with the Media one.
Could we have only one component name but detect thanks to the props what behavior to apply?
This is what https://github.com/ReactTraining/react-media does.

@ehellman
Copy link
Owner

We should think about if this is something that should be in the lib in the first place, since the component that we would build here is exactly the same component that a user of this library could build herself/himself.

But if you guys think it's something that will be used and it's a simpler API, then of course we should build it!

@ValentinH
Copy link
Contributor

I agree with you, in my app, we built to components similar to that: DesktopOnly and MobileOnly.

This is easy to do and flexible so I wouldn't include this in the lib. However, this could be added to the doc for beginners. :)

@joetidee
Copy link
Contributor Author

I think that it will be used 95% of the time and justifies it being in the library for sure.

@ehellman
Copy link
Owner

@ValentinH Could you please shoot me an e-mail at [email protected] so that we can get in touch :)

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

No branches or pull requests

3 participants