add alert to let user know this action can't be undone
https://linear.app/comm/issue/ENG-1016/add-prompt-to-user-that-sidebar-promotion-cant-be-undone
Details
Details
go to promote thread, click cancel, click yes. Functionality should work as expected.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/chat/settings/thread-settings-promote-sidebar.react.js | ||
|---|---|---|
| 37 ↗ | (On Diff #11552) | Do you really need to define a new callback here? I think this has come up before... if so, and assuming there's nothing quirky about the types here – please be thoughtful about wasteful re-definitions like this going forward, and integrate it into your thinking so it doesn't have to be mentioned in review in the future |
Comment Actions
go to promote thread, click cancel, click yes. Functionality should work as expected.
Could you spend a little more time in the future writing your test plans? At this point, this test plan sounds like we should click cancel and then click yes which probably isn't what we wanted. And please start sentences with capital letters.
| native/chat/settings/thread-settings-promote-sidebar.react.js | ||
|---|---|---|
| 29 ↗ | (On Diff #11552) | This text makes sense, but doesn't sound great. But looking at https://linear.app/comm/issue/ENG-1016/add-prompt-to-user-that-sidebar-promotion-cant-be-undone it seems we're aware of that |
| 37 ↗ | (On Diff #11552) | Agree. It looks like onPress: this.props.promoteSidebar should work just fine |
Comment Actions
This should not have been landed, the copy is not clean English...
I added a comment to ENG-1016, but to be clear this needs to be fixed ASAP.
@palys-swm please never accept a diff with broken English like this. The fact that I flagged it on the task should've been a very strong reason NOT to accept this diff. Or at the very least, to add me to the review!
As a result of this, I've added "anything with copy" to the Diff Review Rules that talk about when I need to included in a diff review.
Going forward, if you ever see copy in a diff, make sure you add me to the review as a blocking reviewer, and highlight the copy that needs to be reviewed. I would really love to get to a place where I'm not necessary to review English, but apparently we're not there as a team...
Comment Actions
I added a comment to ENG-1016, but to be clear this needs to be fixed ASAP.
@palys-swm please never accept a diff with broken English like this. The fact that I flagged it on the task should've been a very strong reason NOT to accept this diff. Or at the very least, to add me to the review!
As a result of this, I've added "anything with copy" to the Diff Review Rules that talk about when I need to included in a diff review.
Going forward, if you ever see copy in a diff, make sure you add me to the review as a blocking reviewer, and highlight the copy that needs to be reviewed. I would really love to get to a place where I'm not necessary to review English, but apparently we're not there as a team...
Ok, that makes sense. It seems I got a little confused... sorry for that. Going forward, every time a copy changes, will add @ashoat as a blocking reviewer.
Comment Actions
This is also on me as a native English speaker. For reference, most of the onClick hander was auto-generated with GH autopilot and I didn't take the extra minute to really look at the copy, triple check it, read it out loud. Make sure it was correct.