Skip to content
This repository has been archived by the owner on Feb 12, 2021. It is now read-only.

Add autofill hoc #41

Merged
merged 11 commits into from
Aug 7, 2017
Merged

Add autofill hoc #41

merged 11 commits into from
Aug 7, 2017

Conversation

moriaam
Copy link
Contributor

@moriaam moriaam commented Aug 1, 2017

No description provided.

@moriaam moriaam requested a review from xaviervia August 2, 2017 07:24
@xaviervia xaviervia mentioned this pull request Aug 2, 2017
2 tasks
README.md Outdated
@@ -540,6 +540,26 @@ render(
)
```

## WithAutofillProps (props) (Component)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the w lowercase as per convention :)

return (
<article>
<h1>withAutofillProps</h1>
<input placeholder={autofill ? 'autofilled!' : 'not autofilled'} {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this show? Since auto fill will put context inside. Maybe we can change the border color or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also let's put a name='email' so that the autofill triggers.

injectAutofillHook()

if (window.__klarna_ui_components) {
window.__klarna_ui_components.isAutofillInjected = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s call this magic variable something else, more generic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about __higherOrderComponents__withAutofillProps__wasInjected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rely on document.getElementById(styleElementId) === null instead?

import withAutofillProps from './withAutofillProps'
import { equal } from 'assert'

describe('withAutofillProps', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so about how to test this more, this is what I would do:

  it('has the autofilled prop when autofill animation gets triggered', done => {
    const root = document.createElement('div')
    let animationStartCallback
    function Target ({autoFilled, onAnimationStart}) {
      animationStartCallback = onAnimationStart

      return <div>{autoFilled ? 'autofilled' : 'not autofilled'}</div>
    }

    const DecoratedTarget = withAutoFillProps({
      autofilled: true,
    })(Target)

    render(<DecoratedTarget />, root)

    equal(root.innerText, 'not autofilled')

    animationStartCallback('onAutofillStart')

    equal(root.innerText, 'autofilled')
  })

…and the equivalent for when the autofilled gets removed ( animationStartCallback('onAutofillStop') )

This way you don't have to trigger the real autofill to test it, which is of course impossible 😅

injectStyle(notAutofillHook)
}

function registerAutofill() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have unregisterAutofill as well?

onComponentDidMount() {
  registerAutofill()
}

onComponentWillUnmount() {
  unregisterAutofill()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that safe to do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... if you have 5 components wrapped with this HOC then you can't just remove <style> from DOM when one of them is about to unmount. But from another hand it's a dirty side effect which should be cleaned up somehow if nobody is using it anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to keep track of how many are using it. In global state. Tricky

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it by keeping a counter on the style element instead oh saving it on the window.

@xaviervia xaviervia merged commit 8d03e03 into master Aug 7, 2017
@xaviervia xaviervia deleted the add-autofill-hoc branch August 7, 2017 13:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants