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

Enhance dialog layouts (round 1) #4637

Merged
merged 39 commits into from
Feb 24, 2017
Merged

Enhance dialog layouts (round 1) #4637

merged 39 commits into from
Feb 24, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 22, 2017 •
edited
Loading

Ongoing effort to enhance dialogs with better layouts. (Raised in https://github.com/ethcore/parity/pull/4625). This does not cover all the modals and does not address every thing on each modal - it is part of an ongoing process. (Currently trying to go wide with first-touch before going deep.)

Includes & depends on (these to be merged first, relevant parts in here) -

CreateAccount/Type Selection -

parity 2017-02-22 22-09-14

CreateAccount/Geth import -

parity 2017-02-22 21-30-41
parity 2017-02-22 21-24-01

CreateAccount/Done -

parity 2017-02-23 16-57-26

AddAddress -

parity 2017-02-23 09-12-27

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Feb 22, 2017
@jacogr jacogr changed the title Enhance dialog layouts Enhance dialog layouts (round 1) Feb 23, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 23, 2017
@jacogr jacogr mentioned this pull request Feb 23, 2017
Merged
@jacogr jacogr mentioned this pull request Feb 23, 2017
Merged
@jacogr jacogr added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Feb 23, 2017
@jacogr jacogr removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Feb 24, 2017
@ngotchac
Copy link
Contributor

ngotchac commented Feb 24, 2017 •
edited
Loading

Really like the new layout !

Some small issues though:

  • The imports are not working (JSON File and Presale Wallet)
  • The inputs are not cleaned when you change the Import type (eg. put a name in New Account, go back, click on JSON File : the same name is pre-filled) ; not really an issue but I find it misleading.

One thing that would be great (even though could be outside of this PR), is to use the same upload component that for the Import in Accounts (click or drop). I think it would look especially great in the Portal.

@ngotchac ngotchac added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017 •
edited
Loading

Point #1 - that is not good, I tested all of them. Will re-look.

Point #2 is a good catch - I actually fixed that in the vaults 3 PR. (Could backport that in here - EDIT: ported the single line, not the relevant test)

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 24, 2017
ngotchac
ngotchac reviewed Feb 24, 2017
View reviewed changes
js/src/modals/AddAddress/addAddress.js Outdated
/>
}
onChange={ this.onEditAddress }
ref='inputAddress'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there some refs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't remove that one - it was a toy in dev. Will remove, unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope thinking of another one. No idea, not used there or in the tests, so unsure.

Copy link
Contributor

Choose a reason for hiding this comment

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

So can we remove them ?

Copy link
Contributor Author

@jacogr jacogr Feb 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

They have been. (Unless I didn't push?)

Yeap, didn't push- pushing now.

@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017 •
edited
Loading

Import fix, thanks @ngotchac - https://github.com/ethcore/parity/pull/4674

(Logged https://github.com/ethcore/parity/issues/4675 - would be good to convert file uploads consistently to dropzones)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 24, 2017
@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 24, 2017
@jacogr jacogr merged commit 609e24e into master Feb 24, 2017
@jacogr jacogr deleted the jg-modal-icons branch February 24, 2017 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jacogr @ngotchac