Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix Token Reg Dapp issues in Firefox #4489

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Fix Token Reg Dapp issues in Firefox #4489

merged 4 commits into from
Feb 9, 2017

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Feb 9, 2017

Fixes #4348

@tomusdrw I couldn't reproduce the modal layout issue, but fixed one. Let me know if it still breaks on your side.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M8-dapp 💎 Decentralized applications. labels Feb 9, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017
@tomusdrw
Copy link
Collaborator

tomusdrw commented Feb 9, 2017

Yes, layout looks fine although validation is still not working (Firefox 50.1.0).
To reproduce:

  1. Click "Register a new token"
  2. Enter any 3 characters to "Token TLA"

It will always indicate that TLA should be 3 characters long, bluring input doesn't help.

@ngotchac ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 9, 2017
@@ -105,22 +105,22 @@ export default class InputText extends Component {
const { validationType, contract } = this.props;
const validation = validate(value, validationType, contract);

if (validation instanceof Promise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this wasn't working in Firefox...

Copy link
Contributor

Choose a reason for hiding this comment

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

and we shouldn't do it anyway. there's Promise.resolve and npm i is-promise

Copy link
Contributor

Choose a reason for hiding this comment

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

why doesn't it work in firefox? it's valid JS isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't work reliably with the es6-promise shim, which I'm guessing is included in FF. (Bluebird has the same issue.)

Chrome doesn't show it since it has native Promises.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavofyork In JS, it is considered bad practice to assert instanceof in these cases. We only care about getting a thenable interface, which Promise.resolve should happily accept. See then/is-promise#6

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 9, 2017
@jacogr
Copy link
Contributor

jacogr commented Feb 9, 2017

Issue for the TLA validation - https://github.com/ethcore/parity/issues/4334

disabled: false,
loading: false
});
return Promise.resolve(validation)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

this.setState({ disabled: true, loading: true });
}, 50);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this always feels very hacky, i.e. you are working around something you shouldn't be doing :) Only a mustn't-grumble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... It's more that it's not necessary to change the state twice if the request resolves in less than 50ms, no ?

@jacogr jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017
@jacogr jacogr merged commit 867a593 into master Feb 9, 2017
@jacogr jacogr deleted the ng-fix-token-reg-style branch February 9, 2017 16:41
arkpar pushed a commit that referenced this pull request Mar 3, 2017
* Fix overflow issues in Firefox (#4348)

* Fix wrong Promise inferance

* Add new Componennt for Token Images (#4496)

* Revert "Add new Componennt for Token Images (#4496)"

This reverts commit 6ffbdab.
arkpar added a commit that referenced this pull request Mar 4, 2017
* New chains (#4720)

* Add Kovan chain.

* Fix up --testnet.

* Fix tests.

* Fix test.

* fix test

* Fix test.

* Fix to UglifyJS 2.8.2 to fix app build issues (#4723)

* Update classic bootnodes, ref #4717 (#4735)

* allow failure docker beta

* adjust pruning history default to 64 (#4709)

* backporting from master

[ci-skip]update docker-build.sh

* update gitlab.ci

fix docker hub build
[ci skip]

* update gitlab

docker beta-release->latest

* Add registry.

* Add info on forks.

* Fixed spec file

* Support both V1 & V2 DataChanged events in registry (#4734)

* Add info on forks.

* Add new registry ABI

* Import registry2 & fix exports

* Select ABI based on code hash

* Render new event types (owner not available)

* New registry.

* Rename old chain.

* Fix test.

* Another fix.

* Finish rename.

* Fixed fonts URLs (#4579)

* Fix Token Reg Dapp issues in Firefox (#4489)

* Fix overflow issues in Firefox (#4348)

* Fix wrong Promise inferance

* Add new Componennt for Token Images (#4496)

* Revert "Add new Componennt for Token Images (#4496)"

This reverts commit 6ffbdab.

* Add StackEventListener (#4745)

* Update testnet detection (#4746)

* Fix Account Selection in Signer (#4744)

* Can pass FormattedMessage to Input (eg. Status // RPC Enabled)

* Simple fixed-width fix for Accoutn Selection in Parity Signer
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M8-dapp 💎 Decentralized applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TokenReg validation doesn't work in Firefox
5 participants