Default to the new UI for first time users #5334

whymarrh · 2018-09-24T18:30:28Z

This PR is the start of the new UI switch, showing the new UI by default for first time users.

~~We won't be merging this PR for a while, FYI.~~ This PR is ready for review.

Changes:

Remove instances of "beta" from the old&new UIs
Hide the main announcement for users actively switching back to the old UI
Update end-to-end tests to reflect new defaults

whymarrh · 2018-09-24T21:40:23Z

I'm torn about what we should do with the old UI e2e tests here as updating the e2e tests would require going through the new UI on-boarding to be able to switch back to the old UI. We could delete the old UI e2e tests? It's a non-zero risk though.^{(Is it really? They're pretty flaky anyhow tbf.)}

Similar story for the old UI integration tests. Maybe this PR represents the end of the road for all old UI tests?

tmashuang · 2018-09-24T21:42:45Z

All for deleting the old oldUi e2e tests and integration tests. For the new Ui.

1blockologist · 2018-09-25T00:55:45Z

app/scripts/controllers/preferences.js

@@ -32,7 +32,9 @@ class PreferencesController {
      tokens: [],
      suggestedTokens: {},
      useBlockie: false,
-      featureFlags: {},
+      featureFlags: {
+        betaUI: true,


kind of ironic to push people toward the new UI and still call it betaUI here

What do you mean?

the flag is called "betaUI" and is references throughout the pull request.

when the reality is that it will become the production UI, by nature of being the primary one people get to see and is the natural progression of it.

the idea being that some OTHER unforeseen design become the new betaUI, or the community itself is leveraged to provide skins for metamask

I don't follow, sorry. This is a feature flag that is now permanently enabled as the first step to us removing its usages. It is called betaUI because that's what the feature it was. We will be removing it because, yes, it is becoming the default UI. I don't think that's ironic/unexpected, that's the progression of a feature flag.

If you're interested in community skins, feel free open up a new issue detailing those ideas!

I get it, that's funny. With hindsight, we could have named this parameter something like "ui2" instead. It's like finding an old can of soap in your basement that says "new improved formula!". How new? How beta?

Not a big deal, we can just remove this flag when it's no longer a beta.

whymarrh · 2018-09-27T15:00:38Z

Looks like we'll need to remove update our GH branch settings if we're gonna remove the old UI tests:

tmashuang · 2018-09-27T15:21:57Z

Should we remove the tests in a separate PR?

whymarrh · 2018-09-27T15:23:31Z

Should we remove the tests in a separate PR?

If we make the integration tests non-required we could leave them as failing, yeah? Would that be a better option?

tmashuang · 2018-09-27T15:34:40Z

https://github.com/MetaMask/metamask-extension/blob/538b91c4abd865751e92351d58fa11a74fda73c4/.circleci/config.yml#L151

package.json

tmashuang · 2018-09-27T15:38:39Z

package.json

@@ -13,8 +13,6 @@
    "watch:test:unit": "nodemon --exec \"npm run test:unit\" ./test ./app ./ui",
    "test:unit": "cross-env METAMASK_ENV=test mocha --exit --require test/setup.js --recursive \"test/unit/**/*.js\" \"ui/app/**/*.test.js\" && dot-only-hunter",
    "test:single": "cross-env METAMASK_ENV=test mocha --require test/helper.js",
-    "test:integration": "npm run test:integration:build && npm run test:flat && npm run test:mascara",
-    "test:integration:build": "gulp build:scss",


The issue with this is that the test:ingeration:build is also used for test-integration-mascara

Good catch, thanks

danfinlay · 2018-09-27T16:55:31Z

Interesting Q about the test suite. It's mostly fine to remove old UI tests, except that we could catastrophically break it, and we probably wouldn't notice, since manually testing is just as tedious as it automatically clicking through just to enable the old UI.

That said, the things we are changing right now are just background + new UI, and anything in background that would break the old UI would probably break the new UI too, so maybe we can just be very mindful about breaking background API changes during this interim?

Lastly, how hard would it be for us to copy the new-ui setup to the beginning of the old-UI tests, so we can get the app to a state where they make sense? @tmashuang could you briefly review those files and see if they might compose at all? The old UI tests will probably need some of their first time flow tests removed, since it is no longer exactly possible to encounter.

whymarrh · 2018-10-02T14:54:08Z

Interesting Q about the test suite. It's mostly fine to remove old UI tests, except that we could catastrophically break it, and we probably wouldn't notice

That's a risk, certainly. I would argue that the risk is small though as a lot of functionality/actions are shared and breaking changes would likely be clear during review. I think it's possible to go a single release cycle without old UI e2e tests and without regressions.

kumavis · 2018-10-10T14:36:12Z

Maybe we can run a command from the test to switch to old ui even though there is not a ui option to do it

whymarrh · 2018-10-25T11:30:06Z

@kumavis to follow-up to my suggestions above, I'm thinking we can remove test-integration-flat-chrome and test-integration-flat-firefox from the set of required checks for PRs. I don't think it's worth hacking the old UI tests just to get a green check. The plan is to have the old UI remove completely the release after the one this is is so we're talking about a single release without test coverage for the old UI (which has historically not been a particularly good indicator of anything).

danjm · 2018-11-19T23:39:35Z

test/integration/index.js

@@ -2,7 +2,7 @@ const fs = require('fs')
 const path = require('path')
 const pump = require('pump')
 const browserify = require('browserify')
-const tests = fs.readdirSync(path.join(__dirname, 'lib'))
+const tests = fs.readdirSync(path.join(__dirname, 'lib')) // .filter((filename) => filename.match(/send/))


delete this change

Thanks 😅

More sed magic: sed -i.bak 's/MetaMask Beta/MetaMask/' app/_locales/**/messages.json && \ find app/_locales -name '*.bak' -delete sed -i.bak 's/"welcomeBeta"/"welcome"/' app/_locales/**/messages.json && \ find app/_locales -name '*.bak' -delete

danfinlay

I'm for it. It's very much time!

whymarrh added the DO-NOT-MERGE Pull requests that should not be merged label Sep 24, 2018

whymarrh requested review from danjm and alextsg September 24, 2018 18:30

whymarrh force-pushed the ui-switch branch from 688c3b3 to 98633b8 Compare September 24, 2018 20:51

1blockologist reviewed Sep 25, 2018

View reviewed changes

tmashuang reviewed Sep 27, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

tmashuang reviewed Sep 27, 2018

View reviewed changes

whymarrh force-pushed the ui-switch branch 2 times, most recently from 27e8eca to ae3786a Compare October 2, 2018 14:58

whymarrh mentioned this pull request Oct 9, 2018

Ensure test suite is deterministic #5451

Closed

whymarrh force-pushed the ui-switch branch 2 times, most recently from 23d9324 to 2980059 Compare October 26, 2018 09:20

danjm previously approved these changes Oct 30, 2018

View reviewed changes

whymarrh dismissed danjm’s stale review via 0bb9d8b November 13, 2018 18:07

whymarrh force-pushed the ui-switch branch 6 times, most recently from de08662 to ff235cb Compare November 19, 2018 15:13

danjm reviewed Nov 19, 2018

View reviewed changes

whymarrh force-pushed the ui-switch branch 3 times, most recently from 2446bbf to 39faa35 Compare November 20, 2018 17:20

whymarrh added 11 commits November 20, 2018 14:04

Default to the new UI for first time users

c482c99

Update new UI e2e tests for new UI default

921125b

Add timeout to waitUntilXWindowHandles e2e helper fn

2902fa9

Remove "Beta" from header logo

c5da84d

Update new UI links in the old UI s/Beta/New/

8f8d5d7

Don't announce new UI to users switching back

444c68a

Remove "Beta" from welcome messages

6b32e82

More sed magic: sed -i.bak 's/MetaMask Beta/MetaMask/' app/_locales/**/messages.json && \ find app/_locales -name '*.bak' -delete sed -i.bak 's/"welcomeBeta"/"welcome"/' app/_locales/**/messages.json && \ find app/_locales -name '*.bak' -delete

Remove Mascara from CircleCI config

63e10f6

ci: Drop old UI e2e tests from CircleCI

f1248ac

Speed up genStates script by using built-in require fn

5dc87b6

Remove first-time and navigate-txs integration tests

e68008e

whymarrh force-pushed the ui-switch branch from 39faa35 to e68008e Compare November 20, 2018 17:35

whymarrh added needs review and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 20, 2018

danjm approved these changes Nov 21, 2018

View reviewed changes

tmashuang approved these changes Nov 21, 2018

View reviewed changes

danfinlay approved these changes Nov 21, 2018

View reviewed changes

whymarrh merged commit 66e0de7 into MetaMask:develop Nov 21, 2018

whymarrh deleted the ui-switch branch November 21, 2018 18:51

whymarrh removed the needs review label Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to the new UI for first time users #5334

Default to the new UI for first time users #5334

whymarrh commented Sep 24, 2018 •

edited

Loading

whymarrh commented Sep 24, 2018

tmashuang commented Sep 24, 2018

1blockologist Sep 25, 2018

whymarrh Sep 25, 2018

1blockologist Sep 25, 2018

whymarrh Sep 27, 2018

danfinlay Sep 27, 2018

whymarrh commented Sep 27, 2018

tmashuang commented Sep 27, 2018

whymarrh commented Sep 27, 2018

tmashuang commented Sep 27, 2018

tmashuang Sep 27, 2018 •

edited

Loading

whymarrh Oct 2, 2018

danfinlay commented Sep 27, 2018

whymarrh commented Oct 2, 2018

kumavis commented Oct 10, 2018

whymarrh commented Oct 25, 2018

danjm Nov 19, 2018

whymarrh Nov 19, 2018

danfinlay left a comment

Default to the new UI for first time users #5334

Default to the new UI for first time users #5334

Conversation

whymarrh commented Sep 24, 2018 • edited Loading

whymarrh commented Sep 24, 2018

tmashuang commented Sep 24, 2018

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

whymarrh commented Sep 27, 2018

tmashuang commented Sep 27, 2018

whymarrh commented Sep 27, 2018

tmashuang commented Sep 27, 2018

tmashuang Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danfinlay commented Sep 27, 2018

whymarrh commented Oct 2, 2018

kumavis commented Oct 10, 2018

whymarrh commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danfinlay left a comment

Choose a reason for hiding this comment

whymarrh commented Sep 24, 2018 •

edited

Loading

tmashuang Sep 27, 2018 •

edited

Loading