Adds toggle for primary currency #5421

alextsg · 2018-10-04T01:22:09Z

Fixes #4510
Fixes #5477

Screenshots

bdresser · 2018-10-08T00:22:37Z

looks awesome @alextsg!

@cjeria @danfinlay on the confirm screen, are you cool with the diamond? or would you prefer ETH. Shoulda asked earlier, my b

cjeria · 2018-10-08T21:54:59Z

Conceptually, I like the idea of using a diamond to represent the ETH symbol, however, what I keep on getting caught up with is the fact that it's not exactly the Ethereum symbol - it's generic diamond alt code.

I know some folks in the community have proposed using Ξ to represent Eth. https://www.reddit.com/r/ethereum/comments/6ldlgi/making_%CE%BE_ether_symbol_more_known/

I think if we decide to go with an ETH symbol, we should use the actual version of the logo in SVG format to reuse in different contexts.

Here's a link to an svg version of the eth symbol
https://www.dropbox.com/s/5vkgagjof5vlam1/eth.svg?dl=0

alextsg · 2018-10-08T22:43:34Z

That makes sense, and I'm open to either one. Should we also make the representation, whether it be the ETH logo or Ξ, be consistent across the whole app? This would include the transactions list, wallet view, etc.

whymarrh

I'm not sure introducing a preferences property to the PreferencesController state is the best option here—something about preferencesController.store.getState().preferences seems redundant to me. Shouldn't the entire state of the PreferencesController keep be considered the "preferences"?

Also the generic setPreference method taking a string argument is a bit lax. I could live with it if we introduced constants for each of the available preferences (e.g. setPreference(PreferencesController.USE_ETH_AS_PRIMARY, value)) but I think we're better off having a method to set each of the preferences directly. With this implementation we can easily mistype preference names e.g. setPreference('useETHAsPriamryCurrency', value).

alextsg · 2018-10-09T16:46:30Z

@whymarrh From changes in the PR, preferences would be a property on state.metamask, which is desired because we want to move away from polluting state.metamask with preferences properties. I agree that at first glance preferencesController.store.getState().preferences looks redundant. However, the issue isn't really that the preferences object is being introduced, but that we'll need to refactor the preferences controller to use preferences as its state, because as things are now, properties in the preferences controller state get copied over to state.metamask as top level properties. My goal for this PR was to introduce the preferences object in state.metamask, then refactor and migrate other existing preference props over in a future PR.

I agree with the second point - I was following what we were doing with modals and feature flags. Creating specific methods for each preference will probably work better.

whymarrh · 2018-10-09T19:28:45Z

Ah, yes—I had forgotten that we flatten the state. 😔

bdresser · 2018-10-10T02:30:29Z

Should we also make the representation, whether it be the ETH logo or Ξ, be consistent across the whole app? This would include the transactions list, wallet view, etc.

Yeah, I guess. For now let's use the ethereum diamond here and we can get to the rest shortly. Filed here: #5477

kumavis · 2018-10-10T05:32:50Z

Is this just about Eth vs Fiat or does this also affect displaying token values vs fiat values?

what happens when we're missing fiat values (e.g. ether price api is down)?

kumavis · 2018-10-10T05:36:40Z

@whymarrh @alextsg Can we fix the state flattening issue by starting to use the non-flattened state in the UI?

alextsg · 2018-10-10T05:54:41Z

@kumavis This is about ETH vs Fiat. For tokens this would affect whether an ETH or Fiat value is displayed as the conversion underneath.

By missing Fiat values/price API being down are you referring to the conversionRate in state.metamask? I'm not really sure how that's currently handled in the app, but I think we generally assume that conversionRate is populated properly, and this PR doesn't change that. But since we've been prioritizing Fiat over ETH and this PR would allow users to swap that, I assume missing Fiat values would be less of an issue?

danjm

Nice work! I left a few minor comments.

danjm · 2018-10-15T14:57:00Z

ui/app/components/user-preferenced-token-input/user-preferenced-token-input.component.js

+import PropTypes from 'prop-types'
+import TokenInput from '../token-input'
+
+export default class UserPreferencedCurrencyInput extends PureComponent {


UserPreferencedCurrencyInput -> UserPreferencedTokenInput

danjm · 2018-10-15T15:02:18Z

ui/app/components/currency-input/currency-input.component.js

+    const { decimalValue } = this.state
+
+    return (
+      <UnitInput


Extracting the UnitInput component was a great idea 👍

danjm · 2018-10-15T15:04:54Z

.../user-preferenced-currency-display/tests/user-preferenced-currency-display.component.test.js

+import UserPreferencedCurrencyDisplay from '../user-preferenced-currency-display.component'
+import CurrencyDisplay from '../../currency-display'
+
+describe('UserPreferencedCurrencyInput Component', () => {


UserPreferencedCurrencyInput -> UserPreferencedCurrencyDisplay

danjm · 2018-10-15T15:36:54Z

ui/app/components/user-preferenced-currency-input/user-preferenced-currency-input.container.js

@@ -0,0 +1,13 @@
+import { connect } from 'react-redux'


I wonder if either user-preferenced-currency-input.container.js or currency-input.container.js can be eliminated?

They both only get a small amount of data from state, and the currency-input.container.js is only ever imported in the user-preferenced-currency-input.component.js.

It seems like CurrencyInput could be an unconnected component that just receives its props from its parent. I think this would make it easier to understand the relation between the UserPreferencedCurrencyInput and the CurrencyInput. Also could make CurrencyInput more flexible for future use.

The way I look at this is that UserPreferencedCurrencyInput builds on top of CurrencyInput, which builds on top of UnitInput, the unconnected component. Everything that UserPreferencedCurrencyInput and CurrencyInput does can be done with UnitInput itself, but they're just added layers of convenience and do require being connected to the redux store. It's a much easier API to use for the developer when the component requires just a few props to use and reduces duplication of managing these props.

… and inputs

whymarrh · 2018-10-16T19:12:06Z

@alextsg wanna add "fixes #5477" to the PR description given 91879c4

whymarrh

I did a quick QA pass and this LGTM 👍

alextsg assigned whymarrh and danjm Oct 4, 2018

alextsg requested review from danjm and whymarrh as code owners October 4, 2018 01:22

whymarrh suggested changes Oct 9, 2018

View reviewed changes

bdresser mentioned this pull request Oct 10, 2018

Use the ethereum logo to indicate ETH #5477

Closed

prophit987 previously approved these changes Oct 15, 2018

View reviewed changes

danjm requested changes Oct 15, 2018

View reviewed changes

alextsg added 13 commits October 16, 2018 05:36

Add UnitInput component

65688b9

Add CurrencyInput component

f2d4efd

Add UserPreferencedCurrencyInput component

17f8ef3

Add UserPreferencedCurrencyDisplay component

fb211e7

Add styles for CurrencyInput, CurrencyDisplay, and UnitInput

edffe7a

Add updatePreferences action

3b80e69

Update SettingsTab page with Primary Currency toggle

b8f2c1c

Refactor currency displays and inputs to use UserPreferenced displays…

0d3e3f1

… and inputs

Add TokenInput component

9a3b50e

Add UserPreferencedTokenInput component

20df48e

Use TokenInput in the send screen

61f0caa

Fix unit tests

ede7b41

Fix e2e and integration tests

9943fca

Remove send/CurrencyDisplay component

90d9fb5

alextsg dismissed prophit987’s stale review via 09f7f17 October 16, 2018 06:13

alextsg force-pushed the currency-selector branch 2 times, most recently from 09f7f17 to 91879c4 Compare October 16, 2018 06:25

whymarrh previously approved these changes Oct 16, 2018

View reviewed changes

Replace diamond unicode character with Eth logo. Fix typos

55d7fb5

alextsg dismissed whymarrh’s stale review via 55d7fb5 October 16, 2018 21:56

alextsg force-pushed the currency-selector branch from 91879c4 to 55d7fb5 Compare October 16, 2018 21:56

whymarrh approved these changes Oct 16, 2018

View reviewed changes

danjm approved these changes Oct 16, 2018

View reviewed changes

alextsg merged commit badebe0 into MetaMask:develop Oct 16, 2018

alextsg deleted the currency-selector branch October 16, 2018 23:03

whymarrh mentioned this pull request Oct 17, 2018

optional chainId (extracted from #4932) #5134

Merged

6 tasks

jennypollack mentioned this pull request Jan 21, 2020

option to not show USD? #2807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds toggle for primary currency #5421

Adds toggle for primary currency #5421

alextsg commented Oct 4, 2018 •

edited

Loading

bdresser commented Oct 8, 2018

cjeria commented Oct 8, 2018

alextsg commented Oct 8, 2018

whymarrh left a comment

alextsg commented Oct 9, 2018

whymarrh commented Oct 9, 2018

bdresser commented Oct 10, 2018

kumavis commented Oct 10, 2018

kumavis commented Oct 10, 2018

alextsg commented Oct 10, 2018

danjm left a comment

danjm Oct 15, 2018

danjm Oct 15, 2018

danjm Oct 15, 2018

danjm Oct 15, 2018

alextsg Oct 15, 2018

whymarrh commented Oct 16, 2018

whymarrh left a comment

Adds toggle for primary currency #5421

Adds toggle for primary currency #5421

Conversation

alextsg commented Oct 4, 2018 • edited Loading

bdresser commented Oct 8, 2018

cjeria commented Oct 8, 2018

alextsg commented Oct 8, 2018

whymarrh left a comment

Choose a reason for hiding this comment

alextsg commented Oct 9, 2018

whymarrh commented Oct 9, 2018

bdresser commented Oct 10, 2018

kumavis commented Oct 10, 2018

kumavis commented Oct 10, 2018

alextsg commented Oct 10, 2018

danjm left a comment

Choose a reason for hiding this comment

danjm Oct 15, 2018

Choose a reason for hiding this comment

danjm Oct 15, 2018

Choose a reason for hiding this comment

danjm Oct 15, 2018

Choose a reason for hiding this comment

danjm Oct 15, 2018

Choose a reason for hiding this comment

alextsg Oct 15, 2018

Choose a reason for hiding this comment

whymarrh commented Oct 16, 2018

whymarrh left a comment

Choose a reason for hiding this comment

alextsg commented Oct 4, 2018 •

edited

Loading