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

Start rewriting docs towards queries #119

Merged
merged 6 commits into from
Mar 28, 2019

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Jan 3, 2019

Fixes #118

Motivation

This is one of the tasks before a v2 master release.

Discussion

  • What is the behavior of queries plus child components (i.e. not a child function, but rather child JSX)? Must all queries match, or is it at least one query matches? This bit seems to indicate that it's some as opposed to every.
  • Waiting to see whether query will exist alongside queries

@edorivai
Copy link
Collaborator

edorivai commented Jan 4, 2019

What is the behavior of queries plus child components (i.e. not a child function, but rather child JSX)?

Good question, didn't even think about this actually 🤔. Now that you mention it, the same goes for the render prop... @mjackson have you got an opinion about this?

@edorivai edorivai mentioned this pull request Jan 4, 2019
8 tasks
@gribnoysup
Copy link
Contributor

gribnoysup commented Jan 9, 2019

Maybe my two cents on the topic will help a little as I am responsible for the queries PR 😊

First of all, I tried to preserve the previous behaviour of the render method as much as possible following the discussion in #69

What is the behaviour of queries plus child components ...

This was the "biggest" change in the PR in my opinion.

With query it was pretty straightforward what to do there for the render: if it matches, render children, if it's not, skip rendering. Now when the matches are an object, I decided to go with render even if one matches, as otherwise, it seems pointless to have queries in the first place.

You also can have a case where none of them will match at the same time ever (imagine mixing {min,max}-width for example), so nothing will be rendered ever. I thought that those cases could be really frustrating for some library users.

Hope this clarifies the decision behind some instead of every

Now, with this approach, there is still an issue. If we are rendering children for some cases, how to make children aware of the reason? We can't send those props to a component when it is a DOM element, this will cause React to print nasty warnings (and for a good reason). So instead I decided to pass matches as a prop to children component, but only if it is NOT a DOM element

Waiting to see whether query will exist alongside queries

Per an initial comment in the issue #69 (comment) @mjackson suggested "to release a 2.x that removes the <Media query> prop and uses <Media queries> instead"

Hope that helps!

@edorivai
Copy link
Collaborator

edorivai commented Jan 9, 2019

@mjackson suggested "to release a 2.x that removes the prop and uses instead"

Yeah, I'm aware of this, and when I first read it, I thought it made a lot of sense. However, I came to realize that - at least for my own code bases - this would require quite a lot of refactoring if I'd want to upgrade to v2, without really getting any benefit since most of the time I'm only specifying a single query.

I would actually expect the single-query case to be the most common one, by a significant margin. So given both the smooth transition path, as well as a convenience API for the single-query case, I thought it'd be worth it to consider leaving the query prop alongside queries.

@tstirrat15
Copy link
Contributor Author

Meaning we'd need a PR against the work in #72 to preserve that behavior, yeah?

@edorivai
Copy link
Collaborator

@tstirrat15 Yeah, that's right

contra and others added 2 commits March 17, 2019 16:53
I've been seeing [this comment](ReactTraining#70 (comment)) circulate recently, which is incorrect. react-responsive supports render functions just fine ([documented here](https://github.com/contra/react-responsive#rendering-with-a-child-function)). This behavior was added in 2016 via [this PR](yocontra/react-responsive#64) so it isn't a recent addition.

Not trying to be a jerk here - I don't really care which module people use, but please don't spread misinformation as it just confuses people.

Thanks.
Remove incorrect react-responsive comparison
@tstirrat15
Copy link
Contributor Author

tstirrat15 commented Mar 21, 2019

@edorivai shall I update this towards #123, or do you want to get that merged first?

@edorivai
Copy link
Collaborator

edorivai commented Mar 25, 2019

I've just merged #123 into next, and plan to merge next into master soon. So yeah, I'd say you can start updating towards that.

@tstirrat15
Copy link
Contributor Author

@edorivai have another look. I added a section to Basic Usage with query as an alternative, added a section about the salient differences between the two props, and moved the Render Props documentation to be children of the separate query and queries documentation. It's maybe a little redundant, but I think it's comprehensive.

@edorivai edorivai changed the base branch from master to next March 28, 2019 10:34
@edorivai
Copy link
Collaborator

👍 Love it!

@edorivai edorivai merged commit 5dc1a74 into ReactTraining:next Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants