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

Is withWidth HOC will support class component once useMediaQuery released #14101

Closed
Kaliyani opened this issue Jan 6, 2019 · 23 comments · Fixed by #15678
Closed

Is withWidth HOC will support class component once useMediaQuery released #14101

Kaliyani opened this issue Jan 6, 2019 · 23 comments · Fixed by #15678
Labels
good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@Kaliyani
Copy link
Contributor

Kaliyani commented Jan 6, 2019

As per the meterial-ui document https://material-ui.com/layout/breakpoints/ under the section of JavaScript Media Queries,

withWidth higher-order component will be deprecated for the useMediaQuery hook when the React's hooks are released as stable.

if withWidth HOC deprecated, then how can we acheive the same feature in class component.

@Kaliyani
Copy link
Contributor Author

Kaliyani commented Jan 6, 2019

@oliviertassinari @joshwooding what is your opinion on this.

@joshwooding
Copy link
Member

joshwooding commented Jan 6, 2019

You can create a high order component out of a functional component that uses the useMediaQuery hook. @oliviertassinari maybe we should provide a wrapper in the library

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 6, 2019

We had the same feedback from @anthotsang in #14054. I do agree that we should keep a higher-order component API. But should we keep the withWidth() over a better alternative? I don't know. I do believe that people will no longer write any class component within one or two years.
One possible alternative is to provide a withMediaQuery() HOC.

@joshwooding
Copy link
Member

Or maybe even a render prop API :)

@oliviertassinari oliviertassinari added this to the v4 milestone Jan 6, 2019
@eps1lon
Copy link
Member

eps1lon commented Jan 10, 2019

Just want to note that if we deprecate anything over a hook we deprecate react@^16.3.0 support over react@^16.8.0.

@jacobbogers
Copy link
Contributor

jacobbogers commented Mar 27, 2019

Hi folks,
I would like to have a useWidth that works like so (I already a same hook in #15067)
useMediaQuery is more general tool, I just want to know width expressed as one of the theme breakpoints.

function SomeComponent() {
	const classes = useStyles();
	const theme = useTheme( ); 
	const width = useWidth(theme); // this nice utility function is missing; should return   a string from the `theme.breakpoints.keys` aka 'xs',
        return ....
}

@oliviertassinari oliviertassinari added the new feature New feature or request label Mar 27, 2019
@oliviertassinari oliviertassinari removed this from the v4 milestone Mar 27, 2019
@oliviertassinari
Copy link
Member

Ok, I'm proposing the following plan of action:

  1. We don't deprecate withWidth(), at least, not yet. We remove the notice from https://next.material-ui.com/layout/breakpoints/#withwidth.
  2. We expose useWidth() as a module instead of a demo.

@jacobbogers What do you think? Do you want to submit a pull request for it? 😄

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Mar 27, 2019
@jacobbogers
Copy link
Contributor

Sure olivier, I will make PR, let me read your about process

@jacobbogers
Copy link
Contributor

jacobbogers commented Mar 29, 2019

Guys, I have this use case
In the past i had an issue where components that need to dynamic re-assemble (not only resizing) themselves dependend on size of the browser window.
So since the "width" of the window is a global property why not expose it via a Conext "ProvideWidth"

<ProvideWidth>
  <App />   
</ProvideWidth>

What do you all think of this solution?

@oliviertassinari
Copy link
Member

@jacobbogers What's the win? aside from the extra ceremony required?

@jacobbogers
Copy link
Contributor

jacobbogers commented Mar 29, 2019

@oliviertassinari , my reasoning was motivated by the fact that react itself doesnt use a seperate event handler when you register a function with the "onClick" attribute, there is one event loop. so I was thinking along those same lines that one single event handler listening for media matches and pass it on to components who need it. This would only be uselfull especially if you have many compents needing "width" awareness. Maybe an edge usecase, i had to build a website where a lot of components needed to be aware of the screensize, so thats why i mentioned it. ofc both can be done, have a hook AND a context provider

@oliviertassinari
Copy link
Member

@jacobbogers I hope that React can batch the updates in an efficient way. But I have never tried it. I couldn't tell.

@jacobbogers
Copy link
Contributor

I will make a test, in any case I am working on the hook.

@jacobbogers
Copy link
Contributor

jacobbogers commented Mar 31, 2019

Ok, I have written a useWidth Hook, a WidthProvider and corresponding useWidthContext
doing some benchmark tests https://github.com/jacobbogers/benchmark-mobile.
People can decide what to use depending on their use case.

@joshwooding joshwooding mentioned this issue Apr 28, 2019
29 tasks
@jacobbogers
Copy link
Contributor

Yeah i better wrap this up, lol)

@eps1lon
Copy link
Member

eps1lon commented May 14, 2019

I understand why a useWidth hook is handy for symetry with withWidth. I don't understand why we need a provider for that in the core. Seems like this is better suited for app code.

@jacobbogers
Copy link
Contributor

@eps1lon,
On the "Provider",..., I was reasoning along the lines of react not having dedicated native event handlers (mouseclick) for every component. So just measure (via mediaQueryList) the screensize in one place (the Provider) and pas it down to the many components who need this knowledge.

So it was more of a performance enhancer,

for a single component who needs to know width just use the useWidth directly (no Provider needed), hence I thought it would be a good idea to provide both, and have the dev choose depending on use case.

@oliviertassinari
Copy link
Member

@jacobbogers Did you benchmark the performance difference? Without information, I believe we can ignore it and focus on DX until it's proven to be slow.

@naman13924
Copy link

Kindly create a breakpoint or useMediaQuery functionality for Class Components as well without using higher order component or render props.
React has committed that it will never remove class based components than why people take it that it will get depreciated soon. There are thousands off React apps based on class components.

@oliviertassinari
Copy link
Member

React has committed that it will never remove class based components

@naman13924 Where have you seen this commitment? So far I have seen that they don't plan to touch it for many months. But does it mean forever?

@Magneticmagnum
Copy link

@oliviertassinari from the React docs page on hooks: https://reactjs.org/docs/hooks-intro.html There are no plans to remove classes from React.

@oliviertassinari
Copy link
Member

@Magneticmagnum Definitely, they won't kill it before years, especially if it doesn't cost them much to keep the class API and that a few features don't have equivalences. However, if at some point, 90%of the codebase are on hooks, I'm confident they will. But again, I doubt it will happen before years, if it ever happens.

As long as we can support custom breakpoints, we should be good.

@bmmathe
Copy link

bmmathe commented May 17, 2021

I understand why a useWidth hook is handy for symetry with withWidth. I don't understand why we need a provider for that in the core. Seems like this is better suited for app code.

The reason for adding the hook would be the same reason Material-UI added the useWidth HOC. What I'm having to do now is copy/paste the same useWidth demo hook into all of my new applications which to me indicates it should just be in the framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants