feat(reply): Integrate collaborators endpoint #484
Conversation
mxiao6
commented
May 11, 2020
•
mxiao6
commented
- Fix tests
- More tests
mxiao6
force-pushed
the
collab-mentions-2
branch
2 times, most recently
from
adce7c8 to
228ae85
Compare
| ); | ||
| const PopupList = ({ items, reference, options, ...rest }: Props): JSX.Element => { | ||
| return ( | ||
| <PopupBase className="ba-PopupList" options={{ ...defaultOptions, ...options }} reference={reference}> |
There was a problem hiding this comment.
This will probably cause the popper to be re-rendered constantly due to the object reference changing per-render.
There was a problem hiding this comment.
What was the finding here?
There was a problem hiding this comment.
What was the finding here?
I feel I eventually need to pass in options in the future. But since it's not used in this pr, I will remove it.
src/components/Popups/ReplyField/__tests__/MentionItem-test.tsx
Outdated
Show resolved
Hide resolved
| activeItemIndex?: number; | ||
| itemRowAs?: JSX.Element; | ||
| items: Record<string, unknown>[]; | ||
| onSelect: (index: number, event: React.SyntheticEvent) => void; |
There was a problem hiding this comment.
Is the event used anywhere?
There was a problem hiding this comment.
Is the
eventused anywhere?
not now, but I think we should provide this info to the parent and parent can choose to use it or not.
| editorState: prevEditorState, | ||
| popupReference: null, | ||
| }; | ||
| } | ||
|
|
||
| componentDidMount(): void { | ||
| this.focusEditor(); | ||
| // restore PopupList if it was active | ||
| this.updatePopupReference(); |
There was a problem hiding this comment.
When would this occur?
There was a problem hiding this comment.
When would this occur?
zoom in/out
There was a problem hiding this comment.
But it also has popper position problem when you zoom in/out. I'll remove this line in this pr and address popper issue in an another pr.
| ); | ||
| const PopupList = ({ items, reference, options, ...rest }: Props): JSX.Element => { | ||
| return ( | ||
| <PopupBase className="ba-PopupList" options={{ ...defaultOptions, ...options }} reference={reference}> |
There was a problem hiding this comment.
What was the finding here?
| @@ -0,0 +1,24 @@ | |||
| import { CompositeDecorator, ContentBlock, ContentState, EditorState } from 'draft-js'; | |||
There was a problem hiding this comment.
This file probably move to a more common location, like src/components.
There was a problem hiding this comment.
This file probably move to a more common location, like
src/components.
But it's only used in the ReplyField. Also MentionItem has to move to src/components with it, which may be confusing to people. It makes more sense to me to put it under its only consumer folder.
| <PopupBase className="ba-PopupList" options={{ ...defaultOptions, ...options }} reference={reference}> | ||
| {isEmpty(items) ? ( | ||
| <div className="ba-PopupList-prompt"> | ||
| <FormattedMessage {...messages.popupListPrompt} /> |
There was a problem hiding this comment.
Should there be a separate message for when a user has typed in a filter query but there are no results?
There was a problem hiding this comment.
Should there be a separate message for when a user has typed in a filter query but there are no results?
Waiting for designer's response.
There was a problem hiding this comment.
Rohit said that is ok
src/components/Popups/ReplyField/__tests__/MentionItem-test.tsx
Outdated
Show resolved
Hide resolved
| @@ -1,12 +1,19 @@ | |||
| import * as React from 'react'; | |||
| import React from 'react'; | |||
| import isEmpty from 'lodash/isEmpty'; | |||
There was a problem hiding this comment.
Looks like this is a new import. Is it needed or can we do !items.length?
