pretty self-explanatory. only the primary device should be able to remove other devices. we hide the remove device button if user is not on their primary device.
Details
Details
confirmed that the button only appears if i'm on my primary device
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/profile/linked-devices-list-item.react.js | ||
|---|---|---|
| 20–22 ↗ | (On Diff #43945) | Can't you reuse isPrimary? Their values should be equal |
| native/profile/linked-devices.react.js | ||
| 38–40 ↗ | (On Diff #43945) | Not sure which way of determining primary device is better:
|
| 65 ↗ | (On Diff #43945) | Nit, kinda unrelated: if we need only deviceID, getContentSigningKey() doesn't require Identity context |
| native/profile/linked-devices-list-item.react.js | ||
|---|---|---|
| 39 ↗ | (On Diff #43945) | I'd exit early here Currently, when selected device isn't removable (primary device), an empty bottom sheet is displayed and it looks broken. Instead, it would be better not to open it, or at least display some placeholder text. |
| native/profile/linked-devices.react.js | ||
|---|---|---|
| 25 | Given we only need index to check if the device is primary, I think it would be more readable to determine that at the callsite, and then pass in isPrimary | |
| 41 | You could use the isPrimary variable here | |
| 62–64 | When you have two states that are updated at the same time and read from the same place, it's good practice to combine them into one | |
| native/profile/linked-devices-list-item.react.js | ||
|---|---|---|
| 39 ↗ | (On Diff #43945) | Good catch – yeah, I think we should do this |
Comment Actions
Due to issues in D13125, I ended up checking out this stack locally. While testing I found the experience confusing due to inconsistency between the behavior of primary and non-primary device pills
| native/profile/linked-devices-list-item.react.js | ||
|---|---|---|
| 40–42 ↗ | (On Diff #45274) | It's weird that this button remains pressable even though it does nothing. Can you make the TouchableOpacity no longer pressable when !shouldAllowDeviceRemoval, eg. with disabled={true}? |
| 96 ↗ | (On Diff #45274) | I think it's confusing to the viewer that most of these pills can be pressed, but the one for the primary device can't be. There's no indication to explain why the primary device pill doesn't respond to presses, despite the other pills responding. What do you think of adding a pencil icon to the right of the pills that can be edited? We could use the same SWMansionIcon that we use in EditSettingButton. |
Comment Actions
- Made primary device non-clickable
- Added pencil icons to editable items
- Refactored isThisDevice
- Made type readonly