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

fix usage of idiomatic react props #10

Merged
merged 4 commits into from
Feb 17, 2015
Merged

fix usage of idiomatic react props #10

merged 4 commits into from
Feb 17, 2015

Conversation

ChrisSki
Copy link
Contributor

I think line 49 this.query = toQuery(omit(props, excludedQueryKeys)); was excluding the idiomatic props from being used in toQuery. I was getting Invalid or missing MediaQuery! when not using the query string.

@yocontra
Copy link
Owner

Can you provide an example of where it was breaking?

@ChrisSki
Copy link
Contributor Author

@contra how exactly should I do this? would you like to see how I'm using is in my code?

@yocontra
Copy link
Owner

@ChrisSki The code that was not working. I don't think I'm understanding what the problem was

@ChrisSki
Copy link
Contributor Author

I'll do my best to explain. The following works:

<Mq query="(min-width: 1024px)">
            <div className="actions">
              <div className="action"><span className="actions-create" onClick={this.createNewBoard}>Create New Board</span></div>
              {showUpgrade}
            </div>
          </Mq>

The following does not work:

<Mq minWidth={1100}>
            <div className="actions">
              <div className="action"><span className="actions-create" onClick={this.createNewBoard}>Create New Board</span></div>
              {showUpgrade}
            </div>
          </Mq>

I tried debugging the code and found that when I removed the omit function from this.query = toQuery(omit(props, excludedQueryKeys)); the correct properties, in this case minWidth, work as expected. The only thing I could reason here, was that omit was excluding the properties from mediaQuery.all. I wasn't sure why they would or should be excluded so that was why I tested that case. There is another omit function when declaring props in the render function of index.js and I'm not sure why these are being omitted. However, since this scenario worked for me I didn't remove it.

I attempted other properties that are listed in mediaQuery.js as well, but that did not change the result.

@yocontra
Copy link
Owner

I think the real issue is https://github.com/ChrisSki/react-responsive/blob/idiomatic/src/index.js#L18 needs to be changed to Object.keys(defaultTypes)

@ChrisSki
Copy link
Contributor Author

Looking at it again, I think you're correct. Would you like to fix it or would you like me to submit another PR?

@yocontra
Copy link
Owner

Another PR or updating this one would be fine

change excludedQueryKeys to accept defaultTypes
@ChrisSki
Copy link
Contributor Author

@contra what was actually happening was the types assignment destroyed the initial defaultTypes object before it made it to the updateQuery function. When I moved excludedQueryKeys above types things worked fine. Since types is no longer being used, I removed it.

yocontra pushed a commit that referenced this pull request Feb 17, 2015
fix usage of idiomatic react props
@yocontra yocontra merged commit 0fc6824 into yocontra:master Feb 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants