Feature selector #4074

jacogr · 2017-01-06T16:19:40Z

Feature selector for development mode -

manages switching features on/off under Settings
allows components to query active features via store (and adapt display/execution path accordingly)
currently only used for the i18n selector in dev mode
born out of the need to test features in isolation (e.g. moving ParityBar -> Portal, without de-stabilising non-dev builds)
added tests for all components touched as part of the process

# Conflicts: # js/src/ui/Icons/index.js

coveralls · 2017-01-06T18:25:27Z

Coverage increased (+0.002%) to 86.553% when pulling dbd5723 on jg-dev-features into 9c00eb4 on master.

# Conflicts: # js/src/modals/CreateAccount/NewAccount/newAccount.js # js/src/modals/CreateAccount/NewImport/newImport.js # js/src/modals/CreateAccount/RawKey/rawKey.js # js/src/modals/CreateAccount/RecoveryPhrase/recoveryPhrase.js # js/src/modals/CreateAccount/createAccount.js

coveralls · 2017-01-10T15:31:28Z

Changes Unknown when pulling 7ecae24 on jg-dev-features into ** on master**.

ngotchac · 2017-01-23T18:12:17Z

js/src/ui/Features/features.js

+import React, { Component, PropTypes } from 'react';
+
+import defaults, { MODES } from './defaults';
+import Store from './store';


Not really related to this PR, but I always wondered why we had [module].css, [module].js files, but always (almost) store.js file ? Seems a bit inconsistent no ?

When used internally to the module is it just "THE store". When accesses extrenally, we do it via ui/Features/store so the intent is 100% clear. IMHO option having featureStore doesn't add much if any value.

So what about having styles.css in ui/Features that would contain the Component style ? It would be "THE style" of the Component. I think it solves the issue when you have multiple tabs open with all the same names (even if the path is added in some editors). One way or the other is fine, it just seems a bit strange

ngotchac · 2017-01-23T18:14:03Z

js/src/ui/Features/features.js

+              const onCheck = () => feature.mode === MODES.DEVELOPMENT && this.store.toggleActive(key);
+
+              return (
+                <ListItem


Might want to have separate methods for rendering each item.

ngotchac · 2017-01-23T18:15:12Z

js/src/ui/Features/features.js

+
+export default class Features extends Component {
+  static propTypes = {
+    visible: PropTypes.bool.isRequired


When would this prop be passed ? I would thought it could be read from the local storage so we could have access to it even in Production, no ?

Good point. will take a look.

ngotchac · 2017-01-23T18:22:54Z

js/src/ui/Features/features.js

+                  leftCheckbox={
+                    <Checkbox
+                      checked={ this.store.active[key] }
+                      disabled={ feature.mode === MODES.TESTING }


Not sure I understand the utility of these checkboxes. Aren't they supposed to toggle which features are shown ? Currently they are disabled in DEV mode. Anyhow I think it would be good to have these available in any environment. (would be hidden in PROD of course)

Depends, think it is a bit misleading though -

DEVELOPMENT - it is toggle-able, but off by default - can develop and test without affecting dev mode

TESTING - it is enabled by default, but off for production (in this cse you are probably right, should still be selectable)

PRODUCTION - cannot be switched off, has gone live

ngotchac · 2017-01-23T18:23:33Z

js/src/views/Settings/Parity/parity.js

            { this.renderModes() }
+            <LanguageSelector />
+            <Features />


Looks a bit strange, right in the middle between two features : language and logs. Would be better on top or on bottom

ngotchac

The checkboxes won't toggle (I think it's because an @observer is missing), and it seems that the values written to localStorage gets overwritten on load of the Features component.

ngotchac · 2017-01-24T10:14:58Z

js/src/ui/Features/store.js

+  }
+
+  @action setActiveFeatures = (features = {}, isProduction) => {
+    this.active = Object.assign({}, features, this.getDefaultActive(isProduction));


Shouldn't the defaults be first, not to overwrite the passed features?

There was method to the madness, what it was I don't recall - it has been a bit too long between writing this. Tested specifically for this behaviour -

ui/Features/Store @action setActiveFeatures overrides with defaults:

However, now that dev mode is toggle-able, it would make sense to do it the other way around.

ngotchac · 2017-01-24T10:15:36Z

js/src/ui/Features/features.js

+import Store from './store';
+import styles from './features.css';
+
+export default class Features extends Component {


Missing an @observer here

jacogr added 10 commits January 5, 2017 14:32

WIP

71ba66d

ParityBar verification

63b9fdf

import from index.js

31d042f

Merge branch 'master' into jg-paritybar-update

d5694ae

i18n expansion & tests

f1cf1bd

Features component

05a44dd

Adapt language selector to use features

9d9f5a0

Add features to settings view

0af6735

Merge branch 'master' into jg-dev-features

0f3e87b

# Conflicts: # js/src/ui/Icons/index.js

typo

dbd5723

jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 6, 2017

jacogr added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jan 9, 2017

Merge branch 'master' into jg-dev-features

7ecae24

jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Jan 10, 2017

jacogr added 2 commits January 10, 2017 16:31

Convert logging

07669c6

Merge branch 'master' into jg-dev-features

2e35019

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 Jan 10, 2017

jacogr added 6 commits January 13, 2017 15:59

Merge branch 'master' into jg-dev-features

e3cca92

Merge branch 'master' into jg-dev-features

dccb077

Fix earlier merge issues resulting in test failures

910f5fb

Merge branch 'master' into jg-dev-features

d059fe5

Lint failure fixes (new rules)

666604d

Merge branch 'master' into jg-dev-features

755aede

Merge branch 'master' into jg-dev-features

3cfa0a7

ngotchac reviewed Jan 23, 2017

View reviewed changes

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 Jan 23, 2017

jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 23, 2017

jacogr added 5 commits January 23, 2017 22:17

grumble: split item rendering (& test)

a2330aa

grumble: allow enable/disable while testing (default on)

048ccaa

grumble: move LanguageSelector below Features

40432c2

grumble: don't pass visiblity prop (& update tests)

a59dae5

Merge branch 'master' into jg-dev-features

fd42c5f

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 Jan 23, 2017

ngotchac suggested changes Jan 24, 2017

View reviewed changes

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 Jan 24, 2017

jacogr added 4 commits January 24, 2017 12:02

grumble: missing observable (onClick misbehaving)

9d69ea6

grumble: don't reset to defaults per session

b04e089

Fix to single store instance

77d8900

Merge branch 'master' into jg-dev-features

a26cbda

jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jan 24, 2017

ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 24, 2017

jacogr merged commit 155bbc3 into master Jan 24, 2017

jacogr deleted the jg-dev-features branch January 24, 2017 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature selector #4074

Feature selector #4074

jacogr commented Jan 6, 2017 •

edited

Loading

coveralls commented Jan 6, 2017

coveralls commented Jan 10, 2017

ngotchac Jan 23, 2017

jacogr Jan 23, 2017

ngotchac Jan 24, 2017

ngotchac Jan 23, 2017

ngotchac Jan 23, 2017

jacogr Jan 23, 2017

ngotchac Jan 23, 2017

jacogr Jan 23, 2017 •

edited

Loading

ngotchac Jan 23, 2017

ngotchac left a comment

ngotchac Jan 24, 2017

jacogr Jan 24, 2017 •

edited

Loading

ngotchac Jan 24, 2017

Feature selector #4074

Feature selector #4074

Conversation

jacogr commented Jan 6, 2017 • edited Loading

coveralls commented Jan 6, 2017

coveralls commented Jan 10, 2017

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

jacogr Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacogr commented Jan 6, 2017 •

edited

Loading

jacogr Jan 23, 2017 •

edited

Loading

jacogr Jan 24, 2017 •

edited

Loading