-
Notifications
You must be signed in to change notification settings - Fork 20
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
Comments
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 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 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! :) |
@joetidee about why not using the |
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
Now we would have the ability to import the correct components based on the complexity of the use case. |
@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. |
|
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 |
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:
So it's a bit worse performance wise. |
Why do you feel that there is a need for the |
@joetidee To cover the scenario of when you want the children to only be visible on that single breakpoint, and not breakpoint and up. |
Maybe we just need min/max breakpoint settings to cover every scenario. For example:
|
Are the contributors happy for me to take this on? Are we agreed on a way forward? |
For me this is fine, however I'm not a big fan of the |
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! |
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. :) |
I think that it will be used 95% of the time and justifies it being in the library for sure. |
@ValentinH Could you please shoot me an e-mail at [email protected] so that we can get in touch :) |
Is there any reason why this package isn't written so that it can be used like this:
So that the children are conditionally rendered?
I'm a little confused by your render props example in the readme file, should it be:
As surely the
breakpoints
are captured in theMedia
component using context andcurrentBreakpoint
is calculated byMedia
?The text was updated successfully, but these errors were encountered: