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

Change default value of 'values' prop #30

Merged
merged 2 commits into from
Feb 20, 2020

Conversation

xanido
Copy link
Contributor

@xanido xanido commented Jul 10, 2019

If a consumer doesn't pass values the component should match queries using window.matchMedia on initialisation, if available. However, because the default value of values prop is {}, which is truthy, the component tries to match against an empty object instead.

Conditional check is here: https://github.com/xanido/react-media-query-hoc/blob/b86628/src/media-query-provider.js#L21

@xanido
Copy link
Contributor Author

xanido commented Jul 11, 2019

Should add - this is only necessary because the media state is copied in to a member field in the constructor. This member field is then used and updated in mediaQueryListener, but not in componentDidMount. Not sure if that is deliberate?

@jooj123
Copy link
Contributor

jooj123 commented Jul 30, 2019

had a chat offline - will wait for the other fix for this :)

@gwn
Copy link

gwn commented Sep 21, 2019

Hello, just curious, can you share what the "other fix" is, if not a secret? :)

@jooj123
Copy link
Contributor

jooj123 commented Oct 4, 2019

@gwn i believe @xanido was going to clean up the code so we dont rely on currentMediaState - I have pinged him, hopefully he can reply

@gwn
Copy link

gwn commented Oct 7, 2019

Thanks for the reply. Though it seems that it's been a while since this conversation has started. I think this package is very nice and I would like to see this bug fixed asap so I will try to make a pull request soon with an attempt to clean the code myself; hoping that you'll like it.

@xanido
Copy link
Contributor Author

xanido commented Oct 7, 2019

I think the simplest solution for now is to update currentMediaState inside of componentDidMount, which is what I've updated this PR to do.

@xanido
Copy link
Contributor Author

xanido commented Oct 7, 2019

And I now realise that this is exactly what you proposed @gwn, so I think we're all on the same page now!

@jooj123
Copy link
Contributor

jooj123 commented Oct 9, 2019

@xanido a unit test and we should be good to go :)

@albertstill
Copy link
Contributor

For those outside of Domain, this caused an issue on the first mediaQueryListener fire because it used an out of sync currentMediaState that represented the servers estimated device width using values and not the actual devices width. We had mobile devices suddenly thinking they were desktop for example.

Regarding the duplication of currentMediaState and this.state.media. I'd argue the existence of them both and the use of 20s debounce is unnecessary. The mathMedia events only fire when the media query matches changes, for example (min-width: 300px) only changes when the browser window moves between this point, so it's not rapid firing anyway. I'd be in favour of removing debounce and currentMediaState using this.setState(oldState => ({ media: { ...oldstate.media, [queryName]: matches }). This makes the code simpler and less prone to hard to work out bugs like what's happened. Or am I missing something? Was there another reason debounce was added?

@xanido xanido merged commit d29e90e into DomainGroupOSS:master Feb 20, 2020
@gwn
Copy link

gwn commented Feb 20, 2020

Happy to see this merged. Thanks :)

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.

5 participants