Page MenuHomePhabricator
Differential D8274

[web] Use message search context in message search modal
ClosedPublic
Actions

Authored by inka on Jun 21 2023, 5:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 15, 10:59 PM
Unknown Object (File)
Tue, Mar 25, 2:45 PM
Unknown Object (File)
Mar 15 2025, 5:46 PM
Unknown Object (File)
Mar 12 2025, 12:13 AM
Unknown Object (File)
Mar 4 2025, 10:29 AM
Unknown Object (File)
Mar 4 2025, 10:29 AM
Unknown Object (File)
Mar 4 2025, 10:29 AM
Unknown Object (File)
Mar 4 2025, 10:29 AM
View All Files
Subscribers
ashoat
tomek

Details

Reviewers
kamil
• kuba
michal
Commits
rCOMM7f4b6ce88867: [web] Use message search context in message search modal
Summary

issues: https://linear.app/comm/issue/ENG-3466/add-some-state-for-keeping-the-search-query, https://linear.app/comm/issue/ENG-4109/add-search-button-to-search-modal

The query is saved only once the user has pressed "Search".

Test Plan

(for the purpose of recording this I bound the pinned messages bar to message search modal)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka created this revision.Jun 21 2023, 5:38 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 21 2023, 5:38 AM
inka added a parent revision: D8273: [web] Introduce onClearText to Search.Jun 21 2023, 5:38 AM
inka edited the summary of this revision. (Show Details)Jun 21 2023, 5:42 AM
inka edited the test plan for this revision. (Show Details)
inka attached a referenced file: F599595: Screen Recording 2023-06-21 at 14.40.15.mov. (Show Details)
Harbormaster completed remote builds in B20378: Diff 27927.Jun 21 2023, 5:55 AM
inka requested review of this revision.Jun 21 2023, 5:55 AM
michal added a comment.Jun 22 2023, 3:45 AM

Not sure what the status of the "Search" button is, is the plan for it to be implemented in other search modals too, or is it just a one-off? Do you think it would make sense to add it (as an option) to the SearchModal?

web/modals/search/message-search-modal.react.js
212 ↗(On Diff #27927)

Couldn't we pass it directly?

inka added a child revision: D8275: [web] Make pressing enter initiate search.Jun 22 2023, 7:13 AM
inka mentioned this in D8291: [web] Inline MessageSearchModalContent.Jun 22 2023, 7:19 AM
michal added a comment.Jun 23 2023, 2:13 AM

Also, the query won't be cleared if you manually remove text in the textbox (compared to clicking the X button) which I think I would find confusing.

inka added a comment.EditedJun 23 2023, 2:55 AM

Not sure what the status of the "Search" button is, is the plan for it to be implemented in other search modals too, or is it just a one-off? Do you think it would make sense to add it (as an option) to the SearchModal?

Initially I implemented it like this, but because we want to remember the query in the context, using SearchModal made the code so complex, it was unreasonable. This modal is different from other search modals in that it fetches information from the server, not from redux, and it stores some information in the context. So I think it is reasonable that it requires a different implementation.
The search button is needed for this reason - that we are fetching results from the server, and that our message search is a fulltext search (meaning we cannot find words by their prefix or other part), which is not the case with our other searches

Also, the query won't be cleared if you manually remove text in the textbox (compared to clicking the X button) which I think I would find confusing.

I will bring this up with Ted, but it does make sense to me personally

inka updated this revision to Diff 28038.Jun 23 2023, 2:55 AM
inka edited the summary of this revision. (Show Details)

Address review

Harbormaster failed remote builds in B20456: Diff 28038!Jun 23 2023, 3:18 AM
michal added a comment.Jun 23 2023, 3:36 AM

Makes sense, thanks for explaining

web/modals/search/message-search-modal.react.js
218 ↗(On Diff #28038)

I think that can be inlined

michal accepted this revision.Jun 23 2023, 3:37 AM
This revision is now accepted and ready to land.Jun 23 2023, 3:37 AM
inka updated this revision to Diff 28043.Jun 23 2023, 5:28 AM

Inline button

inka mentioned this in D8301: [web] Remember resutls in search modal.Jun 23 2023, 5:45 AM
Harbormaster failed remote builds in B20461: Diff 28043!Jun 23 2023, 5:50 AM
inka updated this revision to Diff 28048.Jun 23 2023, 8:02 AM

Changes due to changes in D8272

Harbormaster failed remote builds in B20466: Diff 28048!Jun 23 2023, 8:25 AM
Harbormaster completed remote builds in B20466: Diff 28048.Jun 26 2023, 7:38 AM
inka updated this revision to Diff 28149.Jun 27 2023, 3:39 AM

Rebase

Harbormaster completed remote builds in B20532: Diff 28149.Jun 27 2023, 3:56 AM
Closed by commit rCOMM7f4b6ce88867: [web] Use message search context in message search modal (authored by • InkaSokolowska). · Explain WhyJun 27 2023, 4:25 AM
This revision was automatically updated to reflect the committed changes.
• InkaSokolowska added a commit: rCOMM7f4b6ce88867: [web] Use message search context in message search modal.
• InkaSokolowska mentioned this in rCOMM6584accda026: [web] Inline MessageSearchModalContent.

Revision Contents
Changeset List

PathSize
web/
19 lines
modals/
search/
19 lines
55 lines

Diff 28156

View Options

web/app.react.js

// @flow // @flow
import classnames from 'classnames'; import classnames from 'classnames';
import * as React from 'react'; import * as React from 'react';
import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js'; import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js';
import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js'; import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js';
import type { CommunityDrawerItemHandler } from './community-drawer-item-handler.react.js';
import type { HandlerProps } from './community-drawer-item-handlers.react.js'; import type { HandlerProps } from './community-drawer-item-handlers.react.js';
import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js'; import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js';
import css from './community-drawer-item.css'; import css from './community-drawer-item.css';
import { import {
getChildren, getChildren,
getExpandButton, getExpandButton,
} from './community-drawer-utils.react.js'; } from './community-drawer-utils.react.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js'; import ThreadAvatar from '../avatars/thread-avatar.react.js';
Show All 9 Lines
function CommunityDrawerItem(props: DrawerItemProps): React.Node { function CommunityDrawerItem(props: DrawerItemProps): React.Node {
const { const {
itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle }, itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle },
paddingLeft, paddingLeft,
expandable = true, expandable = true,
handlerType, handlerType,
} = props; } = props;
const [handler, setHandler] = React.useState({ const [handler, setHandler] = React.useState<CommunityDrawerItemHandler>({
onClick: () => {}, onClick: () => {},
expanded: false, expanded: false,
toggleExpanded: () => {}, toggleExpanded: () => {},
isActive: false,
}); });
const Handler = getCommunityDrawerItemHandler(handlerType); const Handler = getCommunityDrawerItemHandler(handlerType);
const children = React.useMemo( const children = React.useMemo(
() => () =>
getChildren({ getChildren({
expanded: handler.expanded, expanded: handler.expanded,
▲ Show 20 Lines • Show All 71 Lines • Show Last 20 Lines
View Options

web/modals/search/message-search-modal.css

// @flow // @flow
import classnames from 'classnames'; import classnames from 'classnames';
import * as React from 'react'; import * as React from 'react';
import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js'; import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js';
import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js'; import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js';
import type { CommunityDrawerItemHandler } from './community-drawer-item-handler.react.js';
import type { HandlerProps } from './community-drawer-item-handlers.react.js'; import type { HandlerProps } from './community-drawer-item-handlers.react.js';
import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js'; import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js';
import css from './community-drawer-item.css'; import css from './community-drawer-item.css';
import { import {
getChildren, getChildren,
getExpandButton, getExpandButton,
} from './community-drawer-utils.react.js'; } from './community-drawer-utils.react.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js'; import ThreadAvatar from '../avatars/thread-avatar.react.js';
Show All 9 Lines
function CommunityDrawerItem(props: DrawerItemProps): React.Node { function CommunityDrawerItem(props: DrawerItemProps): React.Node {
const { const {
itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle }, itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle },
paddingLeft, paddingLeft,
expandable = true, expandable = true,
handlerType, handlerType,
} = props; } = props;
const [handler, setHandler] = React.useState({ const [handler, setHandler] = React.useState<CommunityDrawerItemHandler>({
onClick: () => {}, onClick: () => {},
expanded: false, expanded: false,
toggleExpanded: () => {}, toggleExpanded: () => {},
isActive: false,
}); });
const Handler = getCommunityDrawerItemHandler(handlerType); const Handler = getCommunityDrawerItemHandler(handlerType);
const children = React.useMemo( const children = React.useMemo(
() => () =>
getChildren({ getChildren({
expanded: handler.expanded, expanded: handler.expanded,
▲ Show 20 Lines • Show All 71 Lines • Show Last 20 Lines
View Options

web/modals/search/message-search-modal.react.js

// @flow // @flow
import classnames from 'classnames'; import classnames from 'classnames';
import * as React from 'react'; import * as React from 'react';
import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js'; import type { CommunityDrawerItemData } from 'lib/utils/drawer-utils.react.js';
import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js'; import { useResolvedThreadInfo } from 'lib/utils/entity-helpers.js';
import type { CommunityDrawerItemHandler } from './community-drawer-item-handler.react.js';
import type { HandlerProps } from './community-drawer-item-handlers.react.js'; import type { HandlerProps } from './community-drawer-item-handlers.react.js';
import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js'; import { getCommunityDrawerItemHandler } from './community-drawer-item-handlers.react.js';
import css from './community-drawer-item.css'; import css from './community-drawer-item.css';
import { import {
getChildren, getChildren,
getExpandButton, getExpandButton,
} from './community-drawer-utils.react.js'; } from './community-drawer-utils.react.js';
import ThreadAvatar from '../avatars/thread-avatar.react.js'; import ThreadAvatar from '../avatars/thread-avatar.react.js';
Show All 9 Lines
function CommunityDrawerItem(props: DrawerItemProps): React.Node { function CommunityDrawerItem(props: DrawerItemProps): React.Node {
const { const {
itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle }, itemData: { threadInfo, itemChildren, hasSubchannelsButton, labelStyle },
paddingLeft, paddingLeft,
expandable = true, expandable = true,
handlerType, handlerType,
} = props; } = props;
const [handler, setHandler] = React.useState({ const [handler, setHandler] = React.useState<CommunityDrawerItemHandler>({
onClick: () => {}, onClick: () => {},
expanded: false, expanded: false,
toggleExpanded: () => {}, toggleExpanded: () => {},
isActive: false,
}); });
const Handler = getCommunityDrawerItemHandler(handlerType); const Handler = getCommunityDrawerItemHandler(handlerType);
const children = React.useMemo( const children = React.useMemo(
() => () =>
getChildren({ getChildren({
expanded: handler.expanded, expanded: handler.expanded,
▲ Show 20 Lines • Show All 71 Lines • Show Last 20 Lines