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

not rendering on mobile #28

Closed
xidedix opened this issue Oct 27, 2017 · 11 comments
Closed

not rendering on mobile #28

xidedix opened this issue Oct 27, 2017 · 11 comments

Comments

@xidedix
Copy link

xidedix commented Oct 27, 2017

It seems like codemirror is intentionally blocked on mobiles.
Why is that?

if (!IS_MOBILE) {
  cm = require('codemirror');
}
@scniro
Copy link
Owner

scniro commented Oct 27, 2017

Hello,

If you peek at #22, I implemented this for server-side rendering support. I borrowed the implementation from the sample implementation available in react-code-mirror.

I don't specifically see why mobile should be excluded as well but figured this was already solved, hence why it's implementation this way. I'm not opposed to supporting mobile devices but would like to learn more about the use case.

Do you have a need to use codemirror on mobile? I'd imagine it wouldn't be very effective to have syntax editing on mobile - but - I suppose the case could be made for it.

Thoughts?

@xidedix
Copy link
Author

xidedix commented Oct 30, 2017

Hi @scniro
First of all, thank you for the quick response and pushing this project forward.

Currently, there is no option to use react-codemirror2 without SSR setup. The main use case is to support mobile browsers, just like the original codemirror does. I know it's experimental, but it works pretty well.

@scniro
Copy link
Owner

scniro commented Oct 30, 2017

@xidedix hey again. Sure I can take the check out for mobile but leave the navigator check in place for server-side rendering. I am currently trying to work through another issue but as soon as that is resolved I'll include this change as well in the next release (hopefully by tomorrow at the latest)

@slorber
Copy link

slorber commented Nov 2, 2017

Hey @scniro

I'm using this project for https://react-reboot.now.sh/

An user reported an error on Android mobile and it's due to this check:
https://twitter.com/trickydisco78/status/925999722938191872

I don't think my tool should be very usable on mobile as coding on a mobile device is not good DX, but at least I'd like to offer mobile visitors a preview of the purpose of the tool that does not show "unexpected error" :D

@slorber
Copy link

slorber commented Nov 2, 2017

I use the controlled input on mobile, and do update the props from parent, triggering componentWillReceiveProps, this.hydrate() and this.editor.setOptions (which fails)

Not sure exactly to understand the purpose on IS_MOBILE but you probably want to add this to the this.hydrate() to be consistent

@slorber
Copy link

slorber commented Nov 2, 2017

For those interested I published react-codemirror2-mobile with the mobile check removed

@scniro
Copy link
Owner

scniro commented Nov 2, 2017

@slorber This is going to be fixed today. Clearly the issue is fresh and I even make mention that I'll be working through this. To publish a new project only a few hours after your first comment on an active thread is jumping the gun a bit, don't you think? I'd strongly suggest un-publishing that and have a small amount of patience.

@slorber
Copy link

slorber commented Nov 2, 2017

@scniro I'm not forking your project to maintain it, I'm just publishing a static version that I can use as a dependency right now, but for sure I'll use your version once you make it work on mobile

I often do that when I want the fix to work right now and it takes me 5min so not a big deal, now you can take your time to fix it properly ;)

scniro added a commit that referenced this issue Nov 2, 2017
@scniro
Copy link
Owner

scniro commented Nov 2, 2017

@slorber Ahh gotcha, I do similar but Ill usually npm pack the source which builds a binary that can be installed locally as if it were from npm.

Anyways, this should be fixed in the 3.0.6 release. Please check it out and let me know if all looks good? If so I'll go ahead and close this out 👍

@slorber
Copy link

slorber commented Nov 2, 2017

thanks it works fine for me

@scniro scniro closed this as completed Nov 2, 2017
@xidedix
Copy link
Author

xidedix commented Nov 4, 2017

LGTM, thank you Sal!

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

No branches or pull requests

3 participants