Save home tab state as a preference #8612
Conversation
| tabIndex: index, | ||
| isActive: numberOfTabs > 1 && index === this.state.activeTabIndex, | ||
| key: index, |
There was a problem hiding this comment.
I believe this isn't respected, per the docs:[1]
Clone and return a new React element using
elementas the starting point. The resulting element will have the original element’s props with the new props merged in shallowly. New children will replace existing children.keyandreffrom the original element will be preserved.
Also, key isn't a prop, it's a reconciling hint, and this 2nd argument is for props.[2]
| <Tabs | ||
| defaultActiveTabIndex={number('Default Active Tab Index', 0, { min: 0 })} | ||
| > |
There was a problem hiding this comment.
This didn't work before and it continues to not work: the default key isn't referenced after the element is mounted.
ui/app/selectors/selectors.js
Outdated
| * Returns the preferences state | ||
| * @param {Object} state - the state object | ||
| * @returns {Object} | ||
| * @deprecated Use {@code getPreferences} instead |
| * @private | ||
| */ | ||
| _findChildByKey (key) { | ||
| return React.Children.toArray(this.props.children).findIndex((c) => this._generateKey(c) === key) |
There was a problem hiding this comment.
This seems pretty convoluted. Generating these keys upon each render and each click, all so that we can re-order tabs and preserve the preferred tab? I don't see why we'd want that property. I don't know that we've ever re-ordered tabs.
There was a problem hiding this comment.
I disagree that it is convoluted.
Generating these keys upon each render
Generating the keys isn't a particularly expensive operation. Iterating the children also isn't particularly expensive, and this method, _findChildByKey, is only run on mount.
and each click
How does this get run on each click?
There was a problem hiding this comment.
I don't know that we've ever re-ordered tabs.
I think it's a useful property, if we ever add a new tab between two tabs or reorder any tabs ever (that are persisted) it is useful for the default to continue working—it isn't the end of the world if not, but it's definitely a useful additional property.
There was a problem hiding this comment.
if you think about how you would recreate this component using functional components, the generation of each key could be memoized. The initial state could be handled using an initial state setter function
const [state, setState] = useState(() => _findChildByKey(props))overall I'm not a huge fan of intercepting children and cloning elements, but I don't think there would be an issue overall with performance with the changes suggested here. I do agree that changing the order of tabs, no matter how rare, should still respect saved preference.
There was a problem hiding this comment.
I'm not a huge fan of intercepting children and cloning elements
Can we avoid it here? I'm open to not doing this if there's a cleaner way.
There was a problem hiding this comment.
Well... it's different in that we wouldn't need to clone anymore. The <Tab> passed in via children would be exactly what's rendered.
There was a problem hiding this comment.
Its preference, to me it is cleaner and uses APIs that are currently recommended and are easily typed with typescript. Cloning elements in typescript just reveals some intrinsic complexity that is not visible in js.
🤷 As I said, I'm fine with the changes as is. This PR didn't introduce the element cloning, and it isn't inherently bad.
There was a problem hiding this comment.
To give a concrete example of why cloning makes the elements more difficult to type: https://github.com/MetaMask/metamask-extension/blob/develop/ui/app/components/ui/tabs/tab/tab.component.js#L41
I was unable to mark two props as required, because they're injected by <Tabs>. So React would throw the warning each render because they were missing from the original element.
There was a problem hiding this comment.
Re: the original comment, we had talked about using the key or name directly in the isActive conditional, but @whymarrh pointed out that we still need to check on mount to assign a default at least.
If we're going to go with the tab name instead of index, I'd suggest using the tab name directly instead of deriving a key from it and using that. It would be more meaningful for any components using this one as well, as the onTabClick function would return a value that has some meaning to the parent component.
There was a problem hiding this comment.
I've updated this to use the name
| @@ -55,6 +55,7 @@ export default class PreferencesController { | |||
| autoLockTimeLimit: undefined, | |||
| showFiatInTestnets: false, | |||
| useNativeCurrencyAsPrimaryCurrency: true, | |||
| defaultHomeActiveTabKey: undefined, | |||
There was a problem hiding this comment.
I'm not sure this is the best place for this state. This isn't really a user preference right now, and if we did want to make it one (e.g. allow the user to configure a default active home tab), we'd still need to store the last active home tab state somewhere for users without that set.
The AppState controller might be a better spot for this.
There was a problem hiding this comment.
I've updated this to use the AppStateController
This changeset adds the default home tab as a preference, persisted to the AppStateController state. This is updated as the user changes tabs.
|
@MetaMask/extension-devs this should be good now |
This PR adds the default home tab as a preference, persisted to the
PreferencesControllerstate. This is updated as the user changes tabs.There are a few different ways this could've been implemented, but I chose to use string keys instead of the tab index so that we could reorder and/or add/remove tabs (in many cases) without breaking the user's default. This will "break" should we remove the desired tab or change its name but I think that's a fine first pass—breaking here is falling back to the 0th index tab.