Details

Reviewers

tomek
atul
• benschac

Commits

rCOMMa5d44d43531c: [web] Introduce `AddMembersItem` component

Summary

Introduce component rendering single user on the list in AddMembersModal. Because I shared some common logic with native, user information are passed in UserListItem type.

GIF presenting the items:

Screenshot_Google Chrome_2022-04-15_130227.gif (334×442 px, 336 KB)

Test Plan

Component can be tested after introducing AddMembersModal

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Apr 15 2022, 3:57 AM

Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 15 2022, 3:57 AM

• jacek edited the summary of this revision. (Show Details)Apr 15 2022, 4:03 AM

• jacek added a parent revision: D3670: [web] Allow passing `Modal` parameters into `SearchModal`.

• jacek added a parent revision: D3669: [web] Redesign `Modal` to match Figma.

• jacek edited the summary of this revision. (Show Details)

• jacek added a child revision: D3740: [web] Introduce `AddMemberGroup` component.Apr 15 2022, 4:12 AM

• jacek added reviewers: tomek, atul, • benschac.Apr 15 2022, 4:51 AM

Harbormaster completed remote builds in B8225: Diff 11509.Apr 15 2022, 8:15 AM

• jacek requested review of this revision.Apr 15 2022, 8:15 AM

tomek requested changes to this revision.Apr 15 2022, 9:03 AM

tomek added inline comments.

web/modals/threads/members/add-members-item.react.js
19 ↗	(On Diff #11509)	Could you explain what happens here? Why `userInfo.alertTitle === undefined` means that user can be added?
28–30 ↗	(On Diff #11509)	The returned value will be different after each render, so we should probably memoize it
44 ↗	(On Diff #11509)	We should prefer `button` - it's more semantic. We can also set `disabled` attribute when `!canBeAdded`
45–46 ↗	(On Diff #11509)	Why do we need two separate classes for them?

This revision now requires changes to proceed.Apr 15 2022, 9:03 AM

• jacek planned changes to this revision.Apr 19 2022, 11:27 AM

• jacek added inline comments.

web/modals/threads/members/add-members-item.react.js
19 ↗	(On Diff #11509)	It requires some explanation, because it's a bit strange. This is how it's currently implemented in React Native. In the mobile app, the callback that adds user is not executed if `UserListItem` have `alertText` set: https://github.com/CommE2E/comm/blob/master/native/components/user-list-user.react.js#L57-L60 These values are set in: https://github.com/CommE2E/comm/blob/master/lib/shared/search-utils.js#L113-L137 In fact, I should have checked `alertText` instead `alertTitle`, so I'll correct it, although both these fields are filled together and it also works correctly now.
28–30 ↗	(On Diff #11509)	Are you sure it will? I did a quick test, and it seems that the value doesn't change between renders. Maybe the `classNames` library has some optimizations?
44 ↗	(On Diff #11509)	I think the best semantic matching tag would be here `li` as it's more an element of the list (although is clickable).
45–46 ↗	(On Diff #11509)	I think it can even be simplified by using, `.addMemberItem > div` selector in CSS. What do you think? Or should I leave a separate class name for the items in such case?

Updates following review comments

Harbormaster completed remote builds in B8437: Diff 11776.Apr 22 2022, 3:42 AM

tomek requested changes to this revision.Apr 25 2022, 6:28 AM

tomek added inline comments.

web/modals/threads/members/add-members-item.react.js
18 ↗	(On Diff #11776)	In fact, I should have checked alertText instead alertTitle, so I'll correct it, although both these fields are filled together and it also works correctly now. You're still using `alertTitle`. Also, I think it will be safer to use `!userInfo.alertText`.
28–30 ↗	(On Diff #11509)	It's better to check their source code. But even if their code does a memoization, it is safer for us to do the same, just to avoid issues when upgrading the library. It doesn't matter any more, as we're no longer using it here.
44 ↗	(On Diff #11509)	If we really want to be correct, we should probably wrap `button` with `li`. But for now `button` sounds like a good enough solution.
45–46 ↗	(On Diff #11509)	It's more maintainable to use classes - we avoid accidental changes. But at this point, it looks like `.addMemberName` and `.addMemberAction` are used but not defined. We can e.g. introduce a new class `.label` and use it instead of `button.addMemberItem > div`.

This revision now requires changes to proceed.Apr 25 2022, 6:28 AM

replace alertTitle with alertText and change css class name to label

web/modals/threads/members/add-members-item.react.js
18 ↗	(On Diff #11776)	Sorry for missing it

Harbormaster completed remote builds in B8505: Diff 11864.Apr 25 2022, 6:58 AM

tomek accepted this revision.Apr 25 2022, 6:58 AM

This revision is now accepted and ready to land.Apr 25 2022, 6:58 AM

rebase

Harbormaster completed remote builds in B8586: Diff 11967.Apr 27 2022, 2:38 AM

rebase

Harbormaster completed remote builds in B8687: Diff 12098.Apr 29 2022, 3:21 AM

Closed by commit rCOMMa5d44d43531c: [web] Introduce `AddMembersItem` component (authored by • jacek). · Explain WhyApr 29 2022, 3:44 AM

This revision was automatically updated to reflect the committed changes.

• jacek added a commit: rCOMMa5d44d43531c: [web] Introduce `AddMembersItem` component.

Diff 11967

View Options

web/modals/threads/members/add-members-item.react.js

// @flow		// @flow

import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';		import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js';		import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';		import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js';		import type { AppState } from '../redux/redux-setup.js';
import {		import {
workerRequestMessageTypes,		workerRequestMessageTypes,
type WorkerRequestMessage,		type WorkerRequestMessage,
type WorkerResponseMessage,		type WorkerResponseMessage,
} from '../types/worker-types.js';		} from '../types/worker-types.js';

Show All 16 Lines	class DatabaseModule {
status: DatabaseStatus;		status: DatabaseStatus;

constructor() {		constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous		const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined		? undefined
: preloadedState.currentUserInfo?.id;		: preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID);		const isSupported = isSQLiteSupported(currentLoggedInUserID);

if (!isSupported) {		if (!isSupported \|\| isDesktopSafari()) {
		tomekUnsubmitted Not Done Inline Actions Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app. tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
		kamilAuthorUnsubmitted Done Inline Actions good point, thanks kamil: good point, thanks
this.status = databaseStatuses.notSupported;		this.status = databaseStatuses.notSupported;
		kamilAuthorUnsubmitted Done Inline Actions Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
		tomekUnsubmitted Not Done Inline Actions Do we change the status later? tomek: Do we change the status later?
		kamilAuthorUnsubmitted Done Inline Actions yes, in `initDBForLoggedInUser()` kamil: yes, in `initDBForLoggedInUser()`
} else {		} else {
this.init();		this.init();
}		}
}		}

init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {		init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress;		this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH);		this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines

View Options

web/modals/threads/members/members-modal.css

// @flow		// @flow

import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';		import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js';		import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';		import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js';		import type { AppState } from '../redux/redux-setup.js';
import {		import {
workerRequestMessageTypes,		workerRequestMessageTypes,
type WorkerRequestMessage,		type WorkerRequestMessage,
type WorkerResponseMessage,		type WorkerResponseMessage,
} from '../types/worker-types.js';		} from '../types/worker-types.js';

Show All 16 Lines	class DatabaseModule {
status: DatabaseStatus;		status: DatabaseStatus;

constructor() {		constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous		const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined		? undefined
: preloadedState.currentUserInfo?.id;		: preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID);		const isSupported = isSQLiteSupported(currentLoggedInUserID);

if (!isSupported) {		if (!isSupported \|\| isDesktopSafari()) {
		tomekUnsubmitted Not Done Inline Actions Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app. tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
		kamilAuthorUnsubmitted Done Inline Actions good point, thanks kamil: good point, thanks
this.status = databaseStatuses.notSupported;		this.status = databaseStatuses.notSupported;
		kamilAuthorUnsubmitted Done Inline Actions Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
		tomekUnsubmitted Not Done Inline Actions Do we change the status later? tomek: Do we change the status later?
		kamilAuthorUnsubmitted Done Inline Actions yes, in `initDBForLoggedInUser()` kamil: yes, in `initDBForLoggedInUser()`
} else {		} else {
this.init();		this.init();
}		}
}		}

init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {		init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress;		this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH);		this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines

View Options

web/theme.css

// @flow		// @flow

import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';		import { DATABASE_WORKER_PATH, SQLJS_FILE_PATH } from './utils/constants.js';
import { isSQLiteSupported } from './utils/db-utils.js';		import { isDesktopSafari, isSQLiteSupported } from './utils/db-utils.js';
import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';		import WorkerConnectionProxy from './utils/WorkerConnectionProxy.js';
import type { AppState } from '../redux/redux-setup.js';		import type { AppState } from '../redux/redux-setup.js';
import {		import {
workerRequestMessageTypes,		workerRequestMessageTypes,
type WorkerRequestMessage,		type WorkerRequestMessage,
type WorkerResponseMessage,		type WorkerResponseMessage,
} from '../types/worker-types.js';		} from '../types/worker-types.js';

Show All 16 Lines	class DatabaseModule {
status: DatabaseStatus;		status: DatabaseStatus;

constructor() {		constructor() {
const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous		const currentLoggedInUserID = preloadedState.currentUserInfo?.anonymous
? undefined		? undefined
: preloadedState.currentUserInfo?.id;		: preloadedState.currentUserInfo?.id;
const isSupported = isSQLiteSupported(currentLoggedInUserID);		const isSupported = isSQLiteSupported(currentLoggedInUserID);

if (!isSupported) {		if (!isSupported \|\| isDesktopSafari()) {
		tomekUnsubmitted Not Done Inline Actions Can we have a const instead of a function that tells if it is Safari? We can avoid having to detect the browser multiple times, as this fact doesn't change through the lifetime of the app. tomek: Can we have a const instead of a function that tells if it is Safari? We can avoid having to…
		kamilAuthorUnsubmitted Done Inline Actions good point, thanks kamil: good point, thanks
this.status = databaseStatuses.notSupported;		this.status = databaseStatuses.notSupported;
		kamilAuthorUnsubmitted Done Inline Actions Naming might be confusing, but at this point database is not supported (we don't have a key to it), it will be supported after main thread will provide encryption key kamil: Naming might be confusing, but at this point database is not supported (we don't have a key to…
		tomekUnsubmitted Not Done Inline Actions Do we change the status later? tomek: Do we change the status later?
		kamilAuthorUnsubmitted Done Inline Actions yes, in `initDBForLoggedInUser()` kamil: yes, in `initDBForLoggedInUser()`
} else {		} else {
this.init();		this.init();
}		}
}		}

init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {		init(encryptionKey?: ?SubtleCrypto$JsonWebKey) {
this.status = databaseStatuses.initInProgress;		this.status = databaseStatuses.initInProgress;
this.worker = new SharedWorker(DATABASE_WORKER_PATH);		this.worker = new SharedWorker(DATABASE_WORKER_PATH);
▲ Show 20 Lines • Show All 77 Lines • Show Last 20 Lines

[web] Introduce `AddMembersItem` component
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 11967

web/modals/threads/members/add-members-item.react.js

web/modals/threads/members/members-modal.css

web/theme.css

[web] Introduce `AddMembersItem` componentClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 11967

web/modals/threads/members/add-members-item.react.js

web/modals/threads/members/members-modal.css

web/theme.css

[web] Introduce `AddMembersItem` component
ClosedPublic
Actions

Revision Contents
Changeset List