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 modal popup mobile view #209

Merged
merged 2 commits into from
Oct 30, 2017
Merged

Fix modal popup mobile view #209

merged 2 commits into from
Oct 30, 2017

Conversation

medied
Copy link
Member

@medied medied commented Oct 30, 2017

@santisiri after considering and partially trying out other ideas [1], I concluded the best way to fix the modal unresponsive React links in the mobile view was to use the package react-fastclick. Looks like this is a somewhat common issue, and this solution seemed to be more appropriate.

After testing it out it looks like the react-fastclick works fine. Thing is using the package brought my attention to what seems to be a slightly different bug. Modal renders fine and all links work correctly except 'recover password'. After looking into, it seems to be related to the code logic to render and autoposition the modal. Let me elaborate on that, this is what the form for ForgotPassword.jsx is supposed to look like:

  <form id="recover-password" name="forgot-password-form" onSubmit={this.handleSubmit}>
    <div className="w-clearfix login-field">
      <label htmlFor="name" className="login-label login-label-form">{TAPi18n.__('recovery-email')}</label>
      <img src="/images/mail-closed.png" className="login-icon" alt="mail-closed" />
      <input id="recovery-email" type="text" placeholder={TAPi18n.__('email-sample')} className="w-input login-input" />
      {warning}
    </div>
    <div type="submit" id="recovery-button" className="button login-button" onClick={this.handleSubmit}>
      <div>{TAPi18n.__('continue-password-recovery')}</div>
    </div>
  </form>

Behavior I see is: I click on 'recover password', modal renders for like a fraction of a second, then goes away (easier to visualize using Google Chrome Tools debugger). However if I add at least one more field input <div>:

  <form id="recover-password" name="forgot-password-form" onSubmit={this.handleSubmit}>
    <div className="w-clearfix login-field">
      <label htmlFor="name" className="login-label login-label-form">{TAPi18n.__('recovery-email')}</label>
      <img src="/images/mail-closed.png" className="login-icon" alt="mail-closed" />
      <input id="recovery-email" type="text" placeholder={TAPi18n.__('email-sample')} className="w-input login-input" />
      {warning}
    </div>
    <div className="w-clearfix login-field">
      <label htmlFor="name" className="login-label login-label-form">TESTING, TO REMOVE</label>
      <img src="/images/mail-closed.png" className="login-icon" alt="mail-closed" />
      <input id="recovery-email" type="text" placeholder="TESING, TO REMOVE" className="w-input login-input" />
      {warning}
    </div>
    <div type="submit" id="recovery-button" className="button login-button" onClick={this.handleSubmit}>
      <div>{TAPi18n.__('continue-password-recovery')}</div>
    </div>
  </form>

The modal renders and does not go away. With the regular ForgotPassword form, modal height seems to not be correct and somewhere modalPosition() is getting invoked again and messing up the autoPosition.

So a few things:

  • Any insight on how to properly render the modal? Note that this is an issue for mobile only because the desktop view render the 'pointer-up' popup and not the same modal.
  • You think subscribe.js is the best place to initReactFastclick();?

[1] Other options included adding the onTouchStart event listener in every React component with an onClicklistener. Not the most elegant solution in my opinion and, as a matter of fact, not even completely efficient because this brought up a series of double-firing onTouch-onClick mess.

Also looked into adding attributes conditionally but that seemed to bring up the same sort of issues mentioned above.

Some resources I looked at:

JedWatson/react-tappable#41
facebook/react#2055
https://stackoverflow.com/questions/40035230/how-to-conditionally-add-attributes-to-react-dom-element
facebook/react#436 (comment)


Addresses #208

@medied medied mentioned this pull request Oct 30, 2017
@santisiri santisiri merged commit 87fcde2 into master Oct 30, 2017
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.

2 participants