feat: two factor authentication segment in preferences #600
Conversation
gorjan5sk
commented
Jul 14, 2021
gorjan5sk
commented
- Implement two factor authentication segment in Preferences screen, Security tab
- Currently it uses a mocked state implementation providing fake values.
gorjan5sk
force-pushed
the
feature/2fa-prefs-segment
branch
from
a318fc9 to
127e4ea
Compare
| observer(({ appState }) => { | ||
| if (!appState.preferences.isOpen) return null; | ||
| return ( | ||
| <PreferencesView close={() => appState.preferences.closePreferences()} /> |
There was a problem hiding this comment.
The idea behind having shared state is that different components can access it without passing down props to deeply nested components. In this case, we'd just pass appState, instead of passing close
There was a problem hiding this comment.
I'm intentionally decoupling the Preferences here from appState. It'll allow for easier testing of the views, the preferences state and in the future it won't block any potential refactors of AppState and it's dependencies.
| ); | ||
|
|
||
| export interface PreferencesWrapperProps { | ||
| appState: { preferences: { isOpen: boolean; closePreferences: () => void } }; |
There was a problem hiding this comment.
| appState: { preferences: { isOpen: boolean; closePreferences: () => void } }; | |
| appState: AppState; |
| icon="close" | ||
| /> | ||
| </TitleBar> | ||
| <PreferencesCanvas preferences={prefs} /> |
There was a problem hiding this comment.
preferences is shared between PreferencesView, PreferencesCanvas and PaneSelector. Instead of passing it down to the deeply nested PaneSelector, it should be part of Mobx shared state.
There was a problem hiding this comment.
In general, I'm trying to reduce the surface API of React components so that they are more maleable, testable, isolated. Additionally, the current tight coupling between Preferences and TwoFactorAuthentication is only because the TwoFactorAuthentication is a "mocked" state - it provides hardcoded values and will be rewritten upon the integration with the whole system. The decoupling, as I mentioned before, will allow us not only to test the views (which isn't that important), it'll allow us to more precisely test the underlying state management (which is very important).
| export const Security: FunctionComponent<{ prefs: Preferences }> = observer( | ||
| ({ prefs }) => ( | ||
| <PreferencesPane> | ||
| <TwoFactorAuthComponent tfAuth={prefs.twoFactorAuth} /> |
There was a problem hiding this comment.
Same here, instead of reading twoFactorAuth from prefs, then passing it down to TwoFactorAuthComponent, TwoFactorAuthEnabled and TwoFactorAuthDisabled, all components who need access to it should read it from the shared state
| <div className="flex-grow flex flex-col py-6 items-center"> | ||
| <div className="w-125 max-w-125 flex flex-col gap-3">{children}</div> | ||
| </div> | ||
| <div className="flex-basis-55 flex-shrink-max" /> |
There was a problem hiding this comment.
This comment belongs in a StyleKit PR, but why do we need to set flex-shrink to 10000? Won't a flex-shrink with flex-shrink set to 1 do?
There was a problem hiding this comment.
Yep, you're right. I was overcompensating for the feature I wanted :D Fixed now.
| @@ -0,0 +1,27 @@ | |||
| import { Icon, IconType } from '@/components/Icon'; | |||
There was a problem hiding this comment.
For components, let's name the file in camelcase (MenuItem in this case), to keep it consistent with the rest of the project
| @@ -0,0 +1,76 @@ | |||
| import { IconType } from '@/components/Icon'; | |||
There was a problem hiding this comment.
This model is pretty similar to the rest of the models on the shared state, so I think it'd make more sense to keep it in appState too
| @@ -0,0 +1,81 @@ | |||
| import { makeAutoObservable, observable } from 'mobx'; | |||
There was a problem hiding this comment.
Same here, why not keep this model in appState like the rest of them?
gorjan5sk
force-pushed
the
feature/2fa-prefs-segment
branch
from
80b53ab to
eea593b
Compare
