Change query to queries #72

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.

Sign up for GitHub

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

Jump to bottom

Merged

edorivai merged 1 commit into ReactTraining:next from gribnoysup:feature/media-queries

Aug 27, 2018

Contributor

gribnoysup commented Sep 24, 2017

A possible solution for #69

As described in this issue:

query prop is removed
queries prop is added
queries prop values are media queries (and not the prop itself)
render and children props are both functions and render prop is an alias for children (they behave the same way)

I changed tests for browser environment and also added new tests for the server environment.

mjackson requested changes

View reviewed changes

Member

mjackson left a comment

Thanks for the PR! The changes look great, for the most part. I've made a few comments about things I'd like to address before merging.

modules/__tests__/Media-server-test.js Outdated

+                    ReactDOMServer.renderToStaticMarkup(
+                      <Media queries={queries}>
+                        {matches => {
+                          expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Let's make the assertion on the markup instead of in the render prop and change the assertion to be something like "it renders all queries".

modules/__tests__/Media-server-test.js Outdated

+                    ReactDOMServer.renderToStaticMarkup(
+                      <Media queries={queries} defaultMatches={defaultMatches}>
+                        {matches => {
+                          expect(matches).toMatchObject(defaultMatches)

Member

mjackson Oct 23, 2017

Same as above.

modules/__tests__/Media-test.js Outdated

+                      ReactDOM.render(
+                        <Media queries={queries}>
+                          {matches => {
+                            expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Let's test the output of render instead of the arguments to the render prop, just so we have complete coverage, and change the description to be something like "it renders its children function" or similar.

modules/__tests__/Media-test.js Outdated

+                    it('should call children with expected values', () => {
+                      ReactDOM.render(
+                        <Media queries={queries} render={matches => {
+                          expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Same as above.

modules/__tests__/Media-test.js Outdated

+                      ReactDOM.render(
+                        <Media queries={queries}>
+                          {matches => {
+                            expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Same as above.

modules/__tests__/Media-test.js Outdated

+                    it('should call children with expected values', () => {
+                      ReactDOM.render(
+                        <Media queries={queries} render={matches => {
+                          expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Same as above.

modules/__tests__/Media-test.js Outdated

+                      ReactDOM.render(
+                        <Media queries={queries}>
+                          {matches => {
+                            expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Same as above.

modules/__tests__/Media-test.js Outdated

+                    it('should call children with expected values', () => {
+                      ReactDOM.render(
+                        <Media queries={queries} render={matches => {
+                          expect(matches).toMatchObject(expected)

Member

mjackson Oct 23, 2017

Same as above.

modules/Media.js Outdated

                 }
                 render() {
                   const { children, render } = this.props
                   const { matches } = this.state
-                  return (

Member

mjackson Oct 23, 2017

Let's make changes to the rendering logic in a separate PR, please. As written, this change eliminates support for non-function children.

Contributor Author

gribnoysup Oct 23, 2017 •

edited

Loading

Sure! I think I just didn't understand correctly what you meant in the issue comment :)

Contributor Author

gribnoysup commented Oct 24, 2017 •

edited

Loading

I updated the PR, but there are some alterations to the render logic, because beforematches was just a flag, and now it is an object with multiple flags.

I tried to recreate the behavior of the render method as close as possible to the one, that is in the master branch:

render prop not called at all when there are no matches with the queries (same as before)
- render now have matches as a first argument (new)
children component is not rendered at all if there are no matches (same as before)
- we provide matches as a prop only if the child is a composite component (new behavior)
children function always called with matches as a first argument (same as before)

The tests are also updated

Contributor Author

gribnoysup commented Dec 15, 2017

Resolved merge conflicts and prettified everything

gribnoysup closed this

abenhamdine mentioned this pull request

Add <Media queries> prop #69

Closed

flushentitypacket commented Jul 16, 2018

@gribnoysup Was this closed intentionally?

Collaborator

edorivai commented Aug 26, 2018

@gribnoysup I feel like this was closed accidentally, feel free to close again if it was really closed on purpose. @mjackson do you still feel like releasing this as v2? Looking at #69 there's quite some interest in the feature!

edorivai reopened this

Contributor Author

gribnoysup commented Aug 26, 2018 •

edited

Loading

Hi @edorivai! I didn't see any updates for the PR after I made all the changes so I assumed that it is not needed anymore. TBH resolving conflicts with prettier changes in master was hard enough the first time, so if this PR is needed I would rather just do it from scratch


          Change query to queries; Closes ReactTraining#69

634c61f

gribnoysup force-pushed the feature/media-queries branch from 5062135 to 634c61f Compare

August 26, 2018 11:15

Contributor Author

gribnoysup commented Aug 26, 2018 •

edited

Loading

Done, the branch is updated. Hope it wouldn't go stale again 😅

edorivai changed the base branch from master to next

August 27, 2018 09:14

edorivai merged commit 1fb713b into ReactTraining:next

Collaborator

edorivai commented Aug 27, 2018

@gribnoysup thanks a lot! I decided to merge into the next branch for now, as these changes are breaking and we'll probably want to release it as a major version bump.

@mjackson How would you want to move forward from here? Perhaps cut a v2 alpha release?

gribnoysup deleted the feature/media-queries branch

August 27, 2018 10:16

Newnab commented Nov 29, 2018

What's the current status of this? Is it still in the works?

I have a use case where lots of props on a component (in my case, a datepicker) are changed depending on which media queries match, and it would be preferable to me to have a way to have multiple queries in one rather than declaring the component multiple times inside separate components, if possible!

Collaborator

edorivai commented Nov 29, 2018

Hey @Newnab 👋

The status is that we're waiting for @mjackson to jump in and publish the changes to npm. It seems that Michael is a bit busy lately, so I'm not sure when he'll take a look here.

edorivai mentioned this pull request

v1.10 and v2 umbrella #110

Open

8 tasks

oliviertassinari mentioned this pull request

[layout] Add new useMediaQuery hook mui/material-ui#13867

Merged

1 task

tstirrat15 mentioned this pull request

Start rewriting docs towards queries #119

Merged

2 tasks

edorivai mentioned this pull request

[WIP] Backward compatibility for the query prop #123

Merged

2 tasks

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

Labels

None yet