Move asset list to home tab on small screens #8264

Gudahtt · 2020-03-31T20:26:26Z

Two tabs have been created on the home screen: 'Assets' and 'History'. This tabbed view is shown only on small screens (e.g. in the popup). The fullscreen view is unchanged.

The toggle-able left sidebar no longer exists, so some 'sidebar-left' specific code and styles have been removed. The button in the menu bar has been removed as well.

The 'History' title of the transaction history is now redundant when where are no pending transactions, so it as been conditionally hidden.

A passthrough for data-testid has been added to the Tab component for convenience in e2e tests.

metamaskbot · 2020-03-31T20:51:00Z

Builds ready [7a0a279]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (719 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	37	91	48	12	6
		domContentLoaded	617	966	718	83	40
		load	619	968	719	83	40
		domInteractive	617	966	717	83	40

Gudahtt · 2020-03-31T21:58:24Z

Screenshots:

Here is the new popup UI with tabs:

Here's what it looks like when scrolled down somewhat:

Here's what the it looks like at its widest point (beyond which it goes to the fullscreen UI):

Two tabs have been created on the home screen: 'Assets' and 'History'. This tabbed view is shown only on small screens (e.g. in the popup). The fullscreen view is unchanged. The toggle-able left sidebar no longer exists, so some 'sidebar-left' specific code and styles have been removed. The button in the menu bar has been removed as well. The 'History' title of the transaction history is now redundant when where are no pending transactions, so it as been conditionally hidden. A passthrough for `data-testid` has been added to the Tab component for convenience in e2e tests.

metamaskbot · 2020-03-31T22:34:25Z

Builds ready [cb61392]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (679 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Notification	firstPaint	36	119	51	22	11
		domContentLoaded	426	804	677	70	34
		load	427	807	679	70	34
		domInteractive	426	804	677	70	34

Gudahtt · 2020-04-01T00:15:39Z

When you click on a token, it no longer brings you to the transaction history for that token like it used to. I opened #8266 in preparation for updating the Home component to automatically switch to the History tab when a token was selected, but after using it locally I'm not sure I like the idea 🤔. Either way the interaction is not great, but the automatic tab switching does feel a little more weird.

danjm

Reviewed every file, everything looks good. My comments are just what I wrote on the important files after reading them and understanding them. No action is needed.

danjm · 2020-04-01T12:03:06Z

ui/app/components/app/asset-list/asset-list.component.js

@@ -104,7 +89,6 @@ export default class AssetList extends Component {
                name: 'Clicked Token',
              },
            })
-            selectedTokenAddress !== tokenAddress && sidebarOpen && hideSidebar()
          }}
        />
        {this.renderAddToken()}


okay, these file changes just remove the triggering of sidebar hiding from the asset list component. Looks good, makes sense, and explains the below change to the container

danjm · 2020-04-01T12:05:56Z

ui/app/components/app/menu-bar/menu-bar.component.js

-              sidebarOpen ? hideSidebar() : showSidebar()
-            }}
-          />
-        </Tooltip>
        <SelectedAccount />

        <Tooltip


removes the button for opening the sidebar in popup view. looks good, makes sense

danjm · 2020-04-01T12:08:07Z

ui/app/components/app/sidebars/sidebar.component.js

@@ -35,8 +33,6 @@ export default class Sidebar extends Component {
    const { type, sidebarProps = {} } = this.props
    const { transaction = {} } = sidebarProps
    switch (type) {
-      case WALLET_VIEW_SIDEBAR:
-        return <WalletView responsiveDisplayClassname="sidebar-right" />
      case 'customize-gas':


no need for the sidebar component to know about the wallet view if we are no longer showing it in sidebar. Looks good, makes sense

danjm · 2020-04-01T12:15:02Z

ui/app/components/app/transaction-list/transaction-list.component.js

-            { t('history') }
-          </div>
+          {
+            pendingLength > 0 && (


Okay, so the history title is only needed when the "History" tab needs to be divided into pending and history. Makes sense

danjm · 2020-04-01T12:16:13Z

ui/app/components/app/wallet-view/wallet-view.component.js

    history.push(CONNECTED_ROUTE)
-    if (sidebarOpen) {
-      hideSidebar()
-    }
  }



wallet view no longer interacts with a sidebar, doesn't need to know about it 👌, this implies below container changes

danjm · 2020-04-01T12:17:27Z

ui/app/pages/routes/routes.component.js

@@ -250,7 +237,6 @@ export default class Routes extends Component {
          transitionName={sidebarTransitionName}
          type={sidebarType}
          sidebarProps={sidebar.props}
-          onOverlayClose={sidebarOnOverlayClose}
        />
        <NetworkDropdown
          provider={provider}


danjm · 2020-04-01T12:18:34Z

ui/app/components/ui/tabs/tab/tab.component.js

@@ -24,6 +32,7 @@ const Tab = (props) => {

 Tab.propTypes = {
  className: PropTypes.string,
+  'data-testid': PropTypes.string,
  isActive: PropTypes.bool, // required, but added using React.cloneElement
  name: PropTypes.string.isRequired,
  onClick: PropTypes.func,


danjm · 2020-04-01T12:20:49Z

ui/app/pages/home/home.component.js

-                    <TransactionList />
-                  </div>
-                </>
+                  )
              )


refactor isWideViewport logic and add new tabs to home screen, 👌

Gudahtt · 2020-04-01T16:35:02Z

I'm not happy with the UX of this new home page - it's ambiguous whether this is a step forward or not. But it's a step toward designs that are better at least, so I'll merge this for now and we can improve it later.

The `History` title above the transaction history was changed in #8264 to only show when there are pending transactions, because it was redundant to show an additional `History` title below a tab called `History`. It was preserved when there were pending transactions because the pending transactions are shown first in the list, followed by the history, so the title served to divide the two lists. This ended up breaking the fullscreen view though, which doesn't use tabs yet. It has been updated to always show on fullscreen.

Gudahtt force-pushed the home-tabs branch from 2673293 to 7a0a279 Compare March 31, 2020 20:29

Gudahtt marked this pull request as ready for review March 31, 2020 21:58

Gudahtt requested review from danjm, jennypollack and whymarrh as code owners March 31, 2020 21:58

Gudahtt force-pushed the home-tabs branch from 7a0a279 to cb61392 Compare March 31, 2020 22:13

danjm approved these changes Apr 1, 2020

View reviewed changes

Gudahtt merged commit cb0ab90 into develop Apr 1, 2020

danjm mentioned this pull request Apr 1, 2020

Remaining work for LoginPerSite #8248

Closed

18 tasks

Gudahtt mentioned this pull request Apr 1, 2020

Restore History title on wide viewport #8277

Merged

Gudahtt deleted the home-tabs branch April 8, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move asset list to home tab on small screens #8264

Move asset list to home tab on small screens #8264

Gudahtt commented Mar 31, 2020

metamaskbot commented Mar 31, 2020

Gudahtt commented Mar 31, 2020

metamaskbot commented Mar 31, 2020

Gudahtt commented Apr 1, 2020

danjm left a comment

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

danjm Apr 1, 2020

Gudahtt commented Apr 1, 2020

Move asset list to home tab on small screens #8264

Move asset list to home tab on small screens #8264

Conversation

Gudahtt commented Mar 31, 2020

metamaskbot commented Mar 31, 2020

Gudahtt commented Mar 31, 2020

metamaskbot commented Mar 31, 2020

Gudahtt commented Apr 1, 2020

danjm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt commented Apr 1, 2020