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

Better display of tags #4564

Merged
merged 10 commits into from
Feb 16, 2017
Merged

Better display of tags #4564

merged 10 commits into from
Feb 16, 2017

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Feb 15, 2017

This is a WIP

Fixes an issue with the Signer Accounts that were not updating when adding/removing accounts to the Dapps Whitelist.

Would close #4045

Look at the playground: http://localhost:3000/#/playground
Here is what it looks like currently (the bottom tags are hovered):

image

@ngotchac ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Feb 15, 2017
@derhuerst
Copy link
Contributor

Definitely better than before. Tiny details I'd change:

  • remove text-overflow: ellipsis to be able to read more; for it then overflows by a bit unfortunately
  • make .tags 100% of the height & give it overflow-y: scroll
  • decrease the margin: .5em 0 of the tags slightly

name={ name }
unknown
/>
<div className={ styles.mainContainer }>
Copy link
Contributor

Choose a reason for hiding this comment

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

While mucking about - we should really use a Container for these, matches up with what we have everywhere else.

@jacogr
Copy link
Contributor

jacogr commented Feb 15, 2017

On the specific issue mentioned - the APIs are actually being changes (should have the new versions available in the morning - still needs to be reviewed/merged.)

Basically in this view we will start using setNewDappsDefaultAccount instead of setting the full list. (setNewDappsWhitelist also gets an optional default account parameter as to not care about the ordering, making the null, empty list issues elsewhere easier to manage properly.)

@jacogr
Copy link
Contributor

jacogr commented Feb 15, 2017

As for UI -

  1. Hide the tags when not hovered, you can't see them anyway. (Plus we are moving to "extra info on hover" consistently everywhere, so the hints won't mean that much)

  2. Not 100% on-board with the line on the left, would either remove it completely or go full-hog with a different background for the tag container as you had in your pre-mockups.

  3. left/right padding is fine, top/bottom between the 2 tags can be adjusted downwards. (total margin between the 2 should equal padding inside top/bottom of tags.

Apart from that, think I like it.

(General comment, i.e. my comment on use of ~/ui/Container in the code - would like to move the Accounts to use this card before 1.6 is out, so keep that in mind. Alignment with what we have style-wise is important without duplication. Don't duisrupt this one, just keep in mind - that alignment now is optional.)

@ngotchac ngotchac 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 16, 2017
@ngotchac
Copy link
Contributor Author

Okay, I updated it to take your comments into account.

@@ -111,10 +111,16 @@ export default class AccountStore {
}

subscribeDefaultAccount () {
return this._api.subscribe('parity_defaultAccount', (error, defaultAccount) => {
this._api.subscribe('parity_defaultAccount', (error, defaultAccount) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a reason for the return - when testing, you can call the function and do the expectations after promise has resolved. (This way you don't end up with unresolved promises or possible fragile wait-states.)

So revert and rather put the other subscription explicitly in an own function.

@jacogr jacogr 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 16, 2017
@@ -95,20 +94,25 @@ $modalZ: 10001;
}
}

.accountsSection {
padding: 0 1em;
width: 920px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we don't want to take up all available space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it was looking very strange when you switch from 2 accounts displayed to 3 accounts (try it out). The layout would be totally different. Here it is more consistent whatever the number of accounts to show, and it is consistent with the width of the Signer.

@ngotchac ngotchac 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 16, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 16, 2017
@gavofyork gavofyork merged commit b825c8e into master Feb 16, 2017
@gavofyork gavofyork deleted the ng-signer-bar branch February 16, 2017 16:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS issue with the tags in the new account search dialog
4 participants