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

Add SelectionList component to DRY up #4639

Merged
merged 14 commits into from Feb 24, 2017
Merged

Add SelectionList component to DRY up #4639

merged 14 commits into from Feb 24, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 22, 2017 •
edited

  • Add ui/SelectionList
  • Convert ~/modals/DappPermissions & ~/modals/AddDapp removing selection duplication
  • Convert ~/modals/VaultAccounts (addressing code TODO re duplication)
  • Convert ~/views/ParityBar to use selection component
  • Functionality exactly the same, DRY component underneath to take care of selection & default rendering

parity 2017-02-22 17-16-33

parity 2017-02-22 17-16-18

parity 2017-02-22 17-37-30

parity 2017-02-23 10-22-48

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Feb 22, 2017
@derhuerst
Copy link
Contributor

I think left-aligned boxes would give it a cleaner look.

@jacogr
Copy link
Contributor Author

jacogr commented Feb 22, 2017

Actually it is the standard layout for the ~/ui/SectionList component, as used pre-PR-change for the affected selectors. In short, have not mucked about with that.

Happy to have that logged and play around, obviously impacts everywhere that is used - Home, Vaults, these selectors.

@jacogr jacogr mentioned this pull request Feb 22, 2017
Merged
@jacogr jacogr mentioned this pull request Feb 23, 2017
Merged
@jacogr jacogr mentioned this pull request Feb 23, 2017
Merged
ngotchac
ngotchac suggested changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Small comments, one grumble :
I liked the difference of background between a selected only and a default account. It made it a bit more obvious than just the filled star. I really like the new greyscale/colours layout though.

One issue: the account selection layout in the Parity Bar looks a bit broken for me (scroll bar whereas it shouldn't be needed):
image

js/src/modals/DappPermissions/dappPermissions.js Outdated
permissionStore.selectAccount(account.address);
};
onMakeDefault = (account) => {
console.log('onMakeDefault', account);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to get rid of this one.

js/src/modals/DappPermissions/dappPermissions.js Outdated

let className;
onSelect = (account) => {
console.log('onSelect', account);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

js/src/ui/SelectionList/selectionList.css
outline: none;
}
.unselected {
filter: grayscale(100%);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017 •
edited

Will look into the scrollbar issue.

EDIT: Fixed.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 24, 2017
@ngotchac
Copy link
Contributor

One last comment:

I liked the difference of background between a selected only and a default account. It made it a bit more obvious than just the filled star. I really like the new greyscale/colours layout though.

@ngotchac ngotchac added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Feb 24, 2017
@ngotchac ngotchac removed the A0-pleasereview 🤓 Pull request needs code review. label Feb 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017

See it as annoying, but don't have any fixes for it in mind. So feel free to log for enhancement however.

@ngotchac
Copy link
Contributor

All right, will do.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 24, 2017
@jacogr
Copy link
Contributor Author

jacogr commented Feb 24, 2017

Logged https://github.com/ethcore/parity/issues/4666

@jacogr jacogr merged commit a6ed3dc into master Feb 24, 2017
@jacogr jacogr deleted the jg-selectionlist branch February 24, 2017 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac approved these changes

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
3 participants
@jacogr @derhuerst @ngotchac