Refactor removing devices #2881
Conversation
|
@sea-snake please review |
| }: { | ||
| authenticator: DedupAuthenticator; | ||
| index: number; | ||
| i18n: I18n; | ||
| icon?: TemplateResult; | ||
| onRemoveDevice: (device: DeviceData) => void; |
There was a problem hiding this comment.
If the device is passed into authenticatorItem already as argument and there is only a single device passed in, there's no need for the device argument in onRemoveDevice since the parent component already knows the device that should be removed.
On another note, an argument for onRemove would only make sense if the delete button on device A could delete device B e.g. with a dropdown to choose as user.
I would suggest keeping the method abstract:
| onRemoveDevice: (device: DeviceData) => void; | |
| onRemove: () => void; |
There was a problem hiding this comment.
I wasn't thinking on the "best practice" to be honest. Thanks!
| settings.push({ | ||
| action: "remove", | ||
| caption: "Remove", | ||
| fn: () => onRemoveDevice(device), |
There was a problem hiding this comment.
In line with above:
| fn: () => onRemoveDevice(device), | |
| fn: onRemove, |
| @@ -124,26 +127,33 @@ export const authenticatorItem = ({ | |||
| dupCount, | |||
| warn, | |||
| info, | |||
| remove, | |||
| device, | |||
There was a problem hiding this comment.
Unless authenticatorItem needs to render the device, it shouldn't need it as argument.
| device, |
| @@ -96,7 +99,7 @@ export const authenticatorsSection = ({ | |||
| <div class="c-action-list"> | |||
| <ul> | |||
| ${authenticators.map((authenticator, index) => | |||
| authenticatorItem({ authenticator, index, i18n }) | |||
| authenticatorItem({ authenticator, index, i18n, onRemoveDevice }) | |||
There was a problem hiding this comment.
As suggested earlier, from a UI perspective we only have one thing to remove within the authenticatorItem component, so the following should suffice.
| authenticatorItem({ authenticator, index, i18n, onRemoveDevice }) | |
| authenticatorItem({ authenticator, index, i18n, onRemove: () => onRemoveDevice(authenticator. device) }) |
lmuntaner
force-pushed
the
lm-refactor-remove-device
branch
from
02afe04 to
2f7f536
Compare
|
@sea-snake ready for another review |
lmuntaner
force-pushed
the
lm-refactor-remove-device
branch
from
2f7f536 to
89a7cfd
Compare
Motivation
We want to add a confirmation screen instead of an alert when removing devices.
To do that, we'll need to use the same pattern as adding a recovery phrase from the manage page. This means, passing a function that will be triggered with the user interaction instead of calling the function in the Authenticator.
In this PR, I refactor the code so that in a subsequent PR I'll be able to render a new page and then send the user back to the manage page.
Changes
Authenticator:deviceandcanBeRemoved.onRemoveDevicethat callsdeleteDevice.onRemoveDeviceaccordingly.Tests
🟡 Some screens were changed