Update fee card designs to show savings and MM fee by danjm · Pull Request #9629 · MetaMask/metamask-extension

danjm · 2020-10-19T12:20:34Z

Updates the fee card on the view quote screen to match the latest designs.

Edit: screenshots can be seen in this comment #9629 (comment)

This is blocked by #9611

The UI implemented in this PR can stand on its own, but to fully match latest designs, it needs to be combined with #9612

One final missing piece for latest designs is the info tooltip next to savings, which will be implemented in a forthcoming PR

github-actions · 2020-10-19T12:20:52Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2020-10-19T12:52:29Z

Builds ready [d1c40db]

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

Page Load Metrics (434 ± 55 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	111	54	25	12
		domContentLoaded	306	723	432	115	55
		load	307	724	434	115	55
		domInteractive	306	722	432	115	55

metamaskbot · 2020-10-19T13:28:08Z

Builds ready [0a02cbf]

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

Page Load Metrics (772 ± 140 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	389	1357	769	292	140
		load	391	1359	772	292	140
		domInteractive	388	1357	769	292	140

darkwing · 2020-10-19T15:22:38Z

This looks awesome! Quick question -- do we want to synchronize these two percentages? 1% is a bit more than 0.875:

ui/app/pages/swaps/view-quote/view-quote.js

ui/app/pages/swaps/fee-card/fee-card.js

danjm · 2020-10-20T16:29:59Z

@brad-decker This has been rebased and your comments have been addressed

brad-decker

Changes LGTM. I would point out that there is a larger problem in our codebase as a whole about the proper use of HTML tags to match the intent. It wasn't introduced in this PR and progress has been made on some lines of change requests during the review. However, a concerted effort to fix these sort of issues is going to be needed in the future (not specific to swaps or recent work).

metamaskbot · 2020-10-20T16:48:56Z

Builds ready [0a1755d]

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

Page Load Metrics (409 ± 48 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	82	39	14	7
		domContentLoaded	291	584	407	99	48
		load	292	586	409	99	48
		domInteractive	290	584	407	99	48

danjm · 2020-10-20T20:02:14Z

I want to block merging of this on #9662, so that we can properly QA the displayed savings amount. Once that is merged, this will be rebased. I don't think further code changes will be needed.

metamaskbot · 2020-10-22T08:52:33Z

Builds ready [36f268d]

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

Page Load Metrics (554 ± 89 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	308	986	551	184	88
		load	309	991	554	185	89
		domInteractive	308	986	551	184	88

brad-decker · 2020-10-22T20:42:12Z

as I was reviewing/QA'ing #9666 I noticed that there is a mismatch in the sign on the number:

the saving's amount is '-1.63' but in the tooltip it shows '+1.63'... this is

Nope... nope... that's a tilde. That's almost unrecognizable as a tilde

Edit:

in the screenshot, I was able to see it because it's super big compared to what I see on the extension popup on my laptop monitor. @jakehaugen thoughts? I don't think this blocks anything but it threw my old ass eyes for a loop 😆

jakehaugen · 2020-10-23T22:15:33Z

in the screenshot, I was able to see it because it's super big compared to what I see on the extension popup on my laptop monitor. @jakehaugen thoughts? I don't think this blocks anything but it threw my old ass eyes for a loop 😆

Yes, this is an issue with the font we are using. In the mocks I use a different font for the tilde -- Euclid Flex vs. Euclid Circular B. Huge pain. But, I thought we had it solved elsewhere?

brad-decker · 2020-10-23T22:18:16Z

@jakehaugen possibly, i just ran into it when testing. @danjm might have more insight.

metamaskbot · 2020-10-26T15:13:04Z

Builds ready [bf1ca7f]

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

Page Load Metrics (449 ± 49 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	34	122	58	26	12
		domContentLoaded	335	845	447	103	50
		load	336	846	449	103	49
		domInteractive	335	844	447	103	50

danjm · 2020-10-26T17:20:51Z

The tilde has been fixed

danjm · 2020-10-26T17:31:18Z

This was updated to include desired messaging from @jakehaugen when there are no savings:

The base branch was changed.

metamaskbot · 2020-11-09T17:35:11Z

Builds ready [bcd3c93]

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

Page Load Metrics (440 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	88	40	15	7
		domContentLoaded	254	678	438	133	64
		load	256	680	440	132	64
		domInteractive	254	678	438	133	64

darkwing · 2020-11-09T17:44:58Z

The only big issue is seeing the "Terms of Service" link cut off on desktop, which I think is something we should fix -- we shouldn't risk users missing it:

At present, .swaps__container is height: 620px;; could we change that to min-height when in full page mode?

I'm a big concerned about the way we're using space here:

Maybe we could use a sideways arrow (" -> ") to better use the space?

darkwing · 2020-11-09T17:49:53Z

ui/app/pages/swaps/fee-card/fee-card.js

If we initialize savingsText to '', we can void the last else and possibly the if (!inDevelopment) block.

Done in 0c0dffdef

darkwing · 2020-11-09T17:51:33Z

ui/app/pages/swaps/fee-card/index.scss

Might be helpful to add a comment here as to why we're using !important, not sure if we have a standard for doing that with important usage.

Done in 5d6c28567

jakehaugen · 2020-11-09T17:54:36Z

The only big issue is seeing the "Terms of Service" link cut off on desktop, which I think is something we should fix -- we shouldn't risk users missing it:

At present, .swaps__container is height: 620px;; could we change that to min-height when in full page mode?

I'm a big concerned about the way we're using space here:

Maybe we could use a sideways arrow (" -> ") to better use the space?

Will soon be proposing that the Terms of Service link be moved to the first page. There are a few other tweaks in the works along with this. May require a different PR?

darkwing

Added a few suggestions here.

danjm · 2020-11-09T20:36:43Z

@darkwing The terms of service issue was addressed in 7ac38c3b5

Now looks like:

metamaskbot · 2020-11-09T20:53:18Z

Builds ready [5d6c285]

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

Page Load Metrics (432 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	78	38	10	5
		domContentLoaded	248	697	431	142	68
		load	249	699	432	142	68
		domInteractive	248	697	430	142	68

darkwing · 2020-11-10T15:14:47Z

ui/app/pages/swaps/fee-card/fee-card.js

inDevelopment and tokenConversionRate are checked in both else-ifs, so I think that could be tightened up, but I don't want to block on it.

I'm going to leave this where it is for now, as we will be removing this variable in the not to distant future

brad-decker · 2020-11-10T16:02:09Z

ui/app/pages/swaps/fee-card/fee-card.js

could we make use of the wrapperClassName/containerClassName prop on the InfoTooltip component instead of manually creating the extra div here?

some day I will do this without you having to ask me about it on a PR 😅

Done in a7d49f2

css touch up More semantic html and remove unnecessary container wrapper Update message for case when there are no savings, in new swaps fee card designs Improve display of tilde in savings designs

…wn on view-quote screen

…oved during rebase

metamaskbot · 2020-11-10T18:22:08Z

Builds ready [1f43058]

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

Page Load Metrics (364 ± 38 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	79	39	14	7
		domContentLoaded	260	584	362	78	38
		load	261	585	364	79	38
		domInteractive	260	584	362	78	38

danjm requested a review from a team as a code owner October 19, 2020 12:20

danjm requested a review from whymarrh October 19, 2020 12:20

danjm force-pushed the update-fee-card-designs branch from f6ed32e to d1c40db Compare October 19, 2020 12:32

danjm force-pushed the update-fee-card-designs branch from d1c40db to 0a02cbf Compare October 19, 2020 13:06

danjm added the area-swaps label Oct 19, 2020

darkwing reviewed Oct 19, 2020

View reviewed changes

ui/app/pages/swaps/view-quote/view-quote.js Outdated Show resolved Hide resolved

brad-decker suggested changes Oct 19, 2020

View reviewed changes

ui/app/pages/swaps/fee-card/fee-card.js Outdated Show resolved Hide resolved

brad-decker suggested changes Oct 19, 2020

View reviewed changes

ui/app/pages/swaps/fee-card/fee-card.js Outdated Show resolved Hide resolved

ui/app/pages/swaps/fee-card/fee-card.js Outdated Show resolved Hide resolved

ui/app/pages/swaps/fee-card/fee-card.js Outdated Show resolved Hide resolved

danjm force-pushed the update-fee-card-designs branch from 0a02cbf to 0a1755d Compare October 20, 2020 16:29

brad-decker previously approved these changes Oct 20, 2020

View reviewed changes

danjm dismissed brad-decker’s stale review via 36f268d October 22, 2020 08:32

danjm force-pushed the update-fee-card-designs branch from 0a1755d to 36f268d Compare October 22, 2020 08:32

danjm changed the base branch from develop to alternative-savings-fix October 22, 2020 08:34

danjm mentioned this pull request Oct 22, 2020

Savings tooltip and column #9666

Closed

danjm mentioned this pull request Oct 23, 2020

Update main-quote-summary designs/styles #9612

Merged

danjm added the T16-deploy-blocker label Oct 26, 2020

Base automatically changed from alternative-savings-fix to develop November 9, 2020 17:09

danjm force-pushed the update-fee-card-designs branch from b14a14b to bcd3c93 Compare November 9, 2020 17:13

danjm marked this pull request as ready for review November 9, 2020 17:23

darkwing reviewed Nov 9, 2020

View reviewed changes

danjm removed the request for review from whymarrh November 9, 2020 17:51

darkwing reviewed Nov 9, 2020

View reviewed changes

darkwing suggested changes Nov 9, 2020

View reviewed changes

darkwing previously approved these changes Nov 10, 2020

View reviewed changes

brad-decker suggested changes Nov 10, 2020

View reviewed changes

danjm added 6 commits November 10, 2020 13:28

Update fee card designs to show savings and MM fee

dfc80f1

css touch up More semantic html and remove unnecessary container wrapper Update message for case when there are no savings, in new swaps fee card designs Improve display of tilde in savings designs

Ensure terms of service is shown when insufficient eth warning is sho…

52690f3

…wn on view-quote screen

Logic simplification in fee-card.js

f412570

Better center info tooltip icons in fee-card

99d513d

Add comment about use of \!important in fee card css

3baa119

Use container class property on info tooltip in fee card

a7d49f2

danjm dismissed darkwing’s stale review via a7d49f2 November 10, 2020 17:36

danjm force-pushed the update-fee-card-designs branch from 5d6c285 to a7d49f2 Compare November 10, 2020 17:36

Remove function call that was made redundant with 980b140 but not rem…

1f43058

…oved during rebase

danjm mentioned this pull request Nov 10, 2020

Revert "Update main-quote-summary designs/styles (#9612)" #9844

Merged

brad-decker approved these changes Nov 10, 2020

View reviewed changes

danjm merged commit d9924ca into develop Nov 13, 2020

danjm deleted the update-fee-card-designs branch November 13, 2020 17:12

github-actions bot locked and limited conversation to collaborators Nov 13, 2020

Conversation

danjm commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 19, 2020

Uh oh!

metamaskbot commented Oct 19, 2020

Uh oh!

metamaskbot commented Oct 19, 2020

Uh oh!

darkwing commented Oct 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danjm commented Oct 20, 2020

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Oct 20, 2020

Uh oh!

danjm commented Oct 20, 2020

Uh oh!

metamaskbot commented Oct 22, 2020

Uh oh!

brad-decker commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakehaugen commented Oct 23, 2020

Uh oh!

brad-decker commented Oct 23, 2020

Uh oh!

metamaskbot commented Oct 26, 2020

Uh oh!

danjm commented Oct 26, 2020

Uh oh!

danjm commented Oct 26, 2020

Uh oh!

metamaskbot commented Nov 9, 2020

Uh oh!

darkwing commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakehaugen commented Nov 9, 2020

Uh oh!

darkwing left a comment

Choose a reason for hiding this comment

Uh oh!

danjm commented Nov 9, 2020

Uh oh!

metamaskbot commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Nov 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

danjm commented Oct 19, 2020 •

edited

Loading

brad-decker commented Oct 22, 2020 •

edited

Loading