Page MenuHomePhabricator

[web] introduce clear all button to add users list
ClosedPublic

Authored by ginsu on Jan 21 2024, 12:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 7:04 AM
Unknown Object (File)
Fri, Nov 15, 3:36 AM
Unknown Object (File)
Sat, Nov 9, 3:56 AM
Unknown Object (File)
Sat, Nov 9, 3:56 AM
Unknown Object (File)
Sat, Nov 9, 2:44 AM
Unknown Object (File)
Sat, Nov 9, 2:41 AM
Unknown Object (File)
Sat, Nov 9, 1:43 AM
Unknown Object (File)
Mon, Nov 4, 7:45 AM
Subscribers

Details

Summary

PLEASE NOTE THAT THIS DIFF AND SUBSEQUENT DIFFS IN THIS STACK WILL NOT BE LANDED UNTIL MORE OF THE REDESIGN IS READY SINCE THIS WILL CAUSE REGRESSIONS IN PROD

This diff introduces the clear all button + clear all functionality for the add users list

Linear task: https://linear.app/comm/issue/ENG-5958/clear-all-selected-functionality

Depends on D10745

Test Plan

Please see the demo video below

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
web/theme.css
403 ↗(On Diff #35899)

Created a linear task to better track this todo:

https://linear.app/comm/issue/ENG-6496/confirm-the-link-light-mode-colors

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 21 2024, 1:16 PM
Harbormaster failed remote builds in B25967: Diff 35899!
ginsu requested review of this revision.Jan 21 2024, 1:20 PM

will make sure ci passes before landing

atul requested changes to this revision.Jan 22 2024, 9:46 AM

Let's move the inline CSS styles to the CSS file

web/settings/relationship/add-users-list.react.js
130–135 ↗(On Diff #35899)

Can we move this over to the CSS file and use eg classNames to pull it in here.

This revision now requires changes to proceed.Jan 22 2024, 9:46 AM
ginsu requested review of this revision.Jan 24 2024, 2:55 PM

Rerequesting review after responding to concerns inline

web/settings/relationship/add-users-list.react.js
130–135 ↗(On Diff #35899)

Noticed we followed this pattern in our codebase where we use the buttonColor prop to update the color of the button rather than through the classname.

Here are a few examples:

https://github.com/CommE2E/comm/blob/master/web/chat/failed-send.react.js#L86-L94

https://github.com/CommE2E/comm/blob/master/web/invite-links/manage/empty-link-content.react.js#L21-L23

https://github.com/CommE2E/comm/blob/master/web/invite-links/manage/existing-link-content.react.js#L30-L37

https://github.com/CommE2E/comm/blob/master/web/settings/relationship/friend-list-row.react.js#L67-L74

I believe that it would be best to go with what I initially proposed to keep this pattern consistent across the codebase, but lmk if you still disagree.

Seems reasonable, thanks for pointing out other places in the codebase that use that pattern.

This revision is now accepted and ready to land.Jan 24 2024, 7:52 PM
This revision was landed with ongoing or failed builds.Feb 15 2024, 1:18 AM
This revision was automatically updated to reflect the committed changes.