accounts-material-ui icon indicating copy to clipboard operation
accounts-material-ui copied to clipboard

Remove fix once it is fixed in std:accounts-ui

Open PolGuixe opened this issue 9 years ago • 10 comments
trafficstars

Remove fix.js once std:accounts-ui is fixed.

https://github.com/studiointeract/accounts-ui/issues/60

PolGuixe avatar Sep 07 '16 15:09 PolGuixe

fixed and removed fix.js

PolGuixe avatar Nov 15 '16 16:11 PolGuixe

Added fix again. It seems it is not completely fixed in std:accounts-ui

Need to investigate more.

PolGuixe avatar Nov 15 '16 16:11 PolGuixe

@timbrandin https://github.com/studiointeract/accounts-ui/issues/60 has been fixed but I am still getting this issue.

I still need https://github.com/Zetoff/accounts-material-ui/develop/fix.js otherwise I get this error:

LoginForm.jsx:204 Uncaught TypeError: Cannot read property 'trim' of undefined
    at LoginForm.handleChange (LoginForm.jsx:204)
    at Field.triggerUpdate (Field.jsx:16)
    at Field.componentDidMount (Field.jsx:21)
    at modules.js?hash=e87bb51…:187268
    at measureLifeCyclePerf (modules.js?hash=e87bb51…:187078)
    at modules.js?hash=e87bb51…:187267
    at CallbackQueue.notifyAll (modules.js?hash=e87bb51…:180089)
    at ReactReconcileTransaction.close (modules.js?hash=e87bb51…:190339)
    at ReactReconcileTransaction.closeAll (modules.js?hash=e87bb51…:181248)
    at ReactReconcileTransaction.perform (modules.js?hash=e87bb51…:181195)

Any idea?

PolGuixe avatar Jan 11 '17 12:01 PolGuixe

The fix causes another error for me: https://github.com/studiointeract/accounts-ui/issues/92. Apparently, the input object hasn't even been created yet in some cases, causing the reference to value to throw an exception.

dschreij avatar Jan 18 '17 18:01 dschreij

Maybe we can add an extra check to see if 'input' exists?

triggerUpdate() {
		const {onChange} = this.props;
                let value;
		if(this.input) value = this.input.value;

		if (value === undefined) {
			value = '';
		} else {
			// do nothing
		}

		if (this.input) {
			onChange({target: {
					value
				}})
		}
	}

This could be done even prettier of course, but I was trying to change as little as possible.

dschreij avatar Jan 18 '17 19:01 dschreij

@dschreij I agree that it can be done even prettier. It's good for now. In the mid-term - I need to find some time to focus and nail this - I would love to completely remove this fix.

I've tested the your proposed fix and and works great. I'll pushed now.

Next time feel free to just do a PR to develop ;)

PolGuixe avatar Jan 19 '17 17:01 PolGuixe

@dschreij please let me know if the current version works for you.

PolGuixe avatar Jan 19 '17 18:01 PolGuixe

@PolGuixe sorry, I might indeed as well have sent you a PR. I am using your atmosphere package now. Can I just update it there or do I need to use this package from Github to test it (I need to first find out then how to add packages from Github in meteor). Thanks for responding this quickly!

dschreij avatar Jan 19 '17 18:01 dschreij

@dschreij the package is now on atmosphere ;) just do meteor update zetoff:accounts-material-ui and it should update to v0.0.11.

PolGuixe avatar Jan 19 '17 18:01 PolGuixe

Yes! It seems fixed now. Thanks a lot!

dschreij avatar Jan 19 '17 22:01 dschreij