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

Align list displays with SectionList (UI consistency) #4621

Merged
merged 22 commits into from
Feb 24, 2017
Merged

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Feb 21, 2017

monosnap 2017-02-21 12-58-30

monosnap 2017-02-21 12-56-48

monosnap 2017-02-21 12-57-18

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Feb 21, 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 21, 2017
@derhuerst
Copy link
Contributor

Not sure I agree with the UX. But that aside, it lags significantly on my laptop.

MacBook Pro (Retina, 13-inch, Early 2015)
Chromium 56.0.2890.0 (64-bit)

@jacogr
Copy link
Contributor Author

jacogr commented Feb 21, 2017

Transitions. Great feedback, will remove those.

@derhuerst
Copy link
Contributor

Also, consider that there won't necessarily be mouse support (hover) on all clients. It's uncommon among devs, but I can imagine people that want to use the UI on their Surface with touch.

jacogr added a commit that referenced this pull request Feb 24, 2017
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.

Few comments. One thing that must be fixed (the tags not working properly).

On last comment : when hovering a line of Accounts (for example) where the number of item is less than the max (eg. 2), even if the mouse is not over an Account, there will be some transformation/animation going on. This is a bit misleading.

@@ -48,7 +51,8 @@ $transitionAll: all 0.75s cubic-bezier(0.23, 1, 0.32, 1);

.hoverOverlay {
background: $backgroundHover;
transform: scale(1, 1);
/*transform: scale(1, 1);*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't they be removed ? (The commented out properties)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it in on purpose - want to revisit the actual animations, so need guidance as to what it was. (There are a couple)


return app.url === 'web'
? '/web'
: `/app/${app.id}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have this piece of logic somewhere else where it could be accessible from every component? I can imagine that it could be useful to have an easy access to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opacity: 0.85;
padding: 0.25em;
transition: all 0.75s cubic-bezier(0.23, 1, 0.32, 1);
/* https://www.binarymoon.co.uk/2014/02/fixing-css-transitions-in-google-chrome/ */
transform: translateZ(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

<Tags
className={ styles.tags }
tags={ tags }
handleAddSearchToken={ handleAddSearchToken }
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work (cannot click on Tags anymore to add them to the Search)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ummm.... Not intended :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

<div
data-tip
data-for={ `owner_${owner.address}` }
data-effect='solid'
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these for again ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You added them :) It is actually quite nice, shows the name info on hover. (Have it on my todos to replication for Vaults)

Copy link
Contributor

@ngotchac ngotchac Feb 24, 2017

Choose a reason for hiding this comment

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

Ah, right (well it's part of react-tooltip)

@@ -33,7 +33,10 @@ export default class Container extends Component {
const { children, onCloseFirstRun, showFirstRun, upgradeStore } = this.props;

return (
<ParityBackground className={ styles.container }>
<ParityBackground
attachDocument
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want this (at least not for every single dapp). Some dapp devs might built their app thinking that the background would be white. There is actually one example of this on Ropsten, which is just some text, black. If the Parity Background is black/dark, the dapp won't render correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Will make the iframe bg white.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. (iframe bg white)

@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 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
@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
Copy link
Contributor

Looks good, only last point remaining:

when hovering a line of Accounts (for example) where the number of item is less than the max (eg. 2), even if the mouse is not over an Account, there is an transformation/animation going on

@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

Not going to fix that, it actually is part of the SectionList component - feel free to give it a stab, but it is maybe annoying, but really not anywhere near the critical path, so I'm not considering it.

I have discussed this - Best work-around I could muster a couple of PRs ago - need to move the unselected sizes down to 29% when hovering the row, keeping it at 33.33% and makes at least Chrome not resize all equally - and then we end up with an issue since overflows start to occur.

Logged, but not addressing here - https://github.com/ethcore/parity/issues/4665

jacogr added a commit that referenced this pull request Feb 24, 2017
* Added SelectionList component for selections

* Use SelectionList in DappPermisions

* AddDapps uses SelectionList

* Fix AccountCard to consistent height

* Convert Signer defaults to SelectionList

* Subtle selection border

* Convert VaultAccounts to SelectionList

* Add tests for SectionList component

* Apply scroll fixes from lates commit in #4621

* Remove unneeded logs

* Remove extra div, fixing ParityBar overflow
@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 jacogr merged commit f670b18 into master Feb 24, 2017
@jacogr jacogr deleted the jg-sectionlists branch February 24, 2017 14:21
jacogr added a commit that referenced this pull request Feb 24, 2017
* Add VaultMeta edit dialog

* Updated (WIP)

* Meta & password edit completed

* Added SelectionList component for selections

* Use SelectionList in DappPermisions

* AddDapps uses SelectionList

* Fix AccountCard to consistent height

* Convert Signer defaults to SelectionList

* Subtle selection border

* Display account vault information

* Allow invalid addresses to display icons (e.g. vaults)

* Display vault on edit meta

* Convert VaultAccounts to SelectionList

* Allow editing of Vault in meta

* Add tests for SectionList component

* Add tests for VaultSelector component

* Auto-focus description field (aligns with #4657)

* Apply scroll fixes from lates commit in #4621

* Remove unneeded logs

* Remove extra div, fixing ParityBar overflow

* Disable save if password don't match

* s/disabled/readOnly/

* string -> bool
jacogr added a commit that referenced this pull request Feb 24, 2017
* Added SelectionList component for selections

* Use SelectionList in DappPermisions

* AddDapps uses SelectionList

* Fix AccountCard to consistent height

* Display type icons in creation dialog

* Complimentary colours

* Convert Signer defaults to SelectionList

* Fix Geth import - actually pass addresses through

* Work from addresses returned via RPC

* Display actual addresses imported (not selected)

* Update tests to cover bug fixed

* Prettyfy Geth import

* Description on selection actions

* SelectionList as entry point

* Update failing tests

* Subtle selection border

* Styling updates for account details

* Add ModalBox summary

* AddAddress updated

* Convert VaultAccounts to SelectionList

* Add tests for SectionList component

* Add tests for ModalBox component

* Re-apply stretch fix

* Apply scroll fixes from lates commit in #4621

* Clear name on switch (test in #4652, not pulling in)

* Remove refs (cleanup)
gavofyork pushed a commit that referenced this pull request Mar 3, 2017
* Render Dapps via SectionList

* Initial rendering of accounts via SectionList

* Width vars

* Allow classNames in certifications & tags

* Overlay of info on hover

* Adjust hover balances

* Large owner icons (align with vaults)

* Consistent block mined at message

* Attach ParityBackground to html

* Adjust page padding to align

* Lint fixes

* Link to different types of addresses

* Make content parts clickable only (a within a)

* Force Chrome hardware acceleration

* Trust the vendors... don't go crazy with transform :)

* Use faster & default transitions

* Add VaultMeta edit dialog

* Updated (WIP)

* Meta & password edit completed

* Added SelectionList component for selections

* Use SelectionList in DappPermisions

* AddDapps uses SelectionList

* Fix AccountCard to consistent height

* Display type icons in creation dialog

* Complimentary colours

* Convert Signer defaults to SelectionList

* Fix Geth import - actually pass addresses through

* Work from addresses returned via RPC

* Display actual addresses imported (not selected)

* Update tests to cover bug fixed

* Prettyfy Geth import

* Description on selection actions

* SelectionList as entry point

* Update failing tests

* Subtle selection border

* Styling updates for account details

* Add ModalBox summary

* AddAddress updated

* Display account vault information

* Allow invalid addresses to display icons (e.g. vaults)

* Display vault on edit meta

* Convert VaultAccounts to SelectionList

* Allow editing of Vault in meta

* Add tests for SectionList component

* Add tests for ModalBox component

* Add tests for VaultSelector component

* Add vaultsOpened in store

* Add ~/ui/Form/VaultSelect

* WIP

* Fix failing tests

* Move account to vault when selected

* Fix circular build deps

* EditMeta uses Form/VaultSelect

* Vault move into meta store (alignment)

* Re-apply stretch fix

* Display vault in account summary

* Add busy indicators to relevant modals

* Auto-focus description field (aligns with #4657)

* Remove extra container (double scrolling)

* Remove unused container style

* Apply scroll fixes from lates commit in #4621

* Remove unneeded logs

* Remove extra div, fixing ParityBar overflow

* Make dapp iframe background white

* Stop event propgation on tag click

* ChangeVault component (re-usable)

* Use ChangeVault component

* Pass vaultStores in

* Icon highlight colour

* Tag-ify vault name display

* ChangeVault location

* Bothced merge, selector rendering twice

* Value can be undefined (no vault)

* Close selector on Select bug

* Fix toggle botched merge

* Update tests

* Add Vault Tags to Account Header
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.

3 participants