feature: implement new ProjectTree design with popout menus #4361
Conversation
…osoft/BotFramework-Composer into beyackle/botProjectTreeOnly
…osoft/BotFramework-Composer into beyackle/botProjectTreeOnly
beyackle
requested review from
a-b-r-o-w-n,
boydc2014,
cwhitten,
GeoffCoxMSFT,
hatpick,
srinaath and
tonyanziano
as code owners
Composer/packages/client/src/recoilModel/dispatchers/__tests__/qna.test.tsx
Show resolved
Hide resolved
Composer/packages/client/src/components/ProjectTree/ProjectTree.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| // TODO: swap to the commented-out block once we're working on skills for real | ||
| // let { skillId } = props; | ||
| // if (skillId == null) skillId = projectId; |
There was a problem hiding this comment.
const { location, dialogId, projectId = '' } = props;
So i dont see references to the skillId being set from the props. Essentially, with this current Design Page how would skills be supported. It seems like the schema, shellApi are all going to be only in the context of the rootBot. Should this be changed to
const { location, dialogId, projectId = '', skillId } = props;
const currentProjectId = skillId || projectId
Now all the useRecoilValues would use something like useRecoilValue(schemasState(currentProjectId))There was a problem hiding this comment.
I left skillId out of the props on purpose because, at this stage, we don't need it yet. This is a minimal change to get the new ProjectTree design in without overcomplicating things. The older branch I'm pulling stuff from did read skillId, just like you're suggesting.
| @@ -173,16 +194,26 @@ const onRenderItem = (item: IOverflowSetItemProps) => { | |||
| /> | |||
| )} | |||
| {item.displayName} | |||
| {item.errorContent && ( | |||
There was a problem hiding this comment.
Are these are the error indicators inside the project tree?
@zhixzhan This might be useful for the work your doing
There was a problem hiding this comment.
@srinaath Sure, It helps.
One more requirements from my side is a missing style.
There was a problem hiding this comment.
Interesting. I won't make that change here, but if I could learn more about the specifics of this I'll make another PR for that. Are skills the only things that can be greyed out? How will the client know if a skill is broken/missing?
There was a problem hiding this comment.
Lets discuss this in the evenings status sync
Composer/packages/client/src/components/ProjectTree/treeItem.tsx
Outdated
Show resolved
Hide resolved
| groupRef.current?.toggleCollapseAll(true); | ||
| props.onToggleCollapse?.(props.group!); | ||
| onSelect(props.group!.key); | ||
| const botHasErrors = (bot: BotInProject) => { |
There was a problem hiding this comment.
nit: You could move it inside the selector. It would be cached once and recalculted only when dialogs change rather than being recalculated multiple times here
There was a problem hiding this comment.
If you want to keep it here, please use useMemo
There was a problem hiding this comment.
I'm not sure about that. useMemo uses reference equality to check if it needs to update, and this is a live view of whether a bot or dialog has an error. If I type something into a trigger's actions that causes an error (like "=[", which doesn't parse), this produces error icons on the trigger and the dialog it belongs to. The work it's doing is only linear in the number of triggers as well, which I don't think is costly enough to warrant useMemo.
Composer/packages/client/src/components/ProjectTree/ExpandableNode.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,93 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
nit: Maybe move this file inside __mocks folder rather than directly inside testUtils
There was a problem hiding this comment.
That's a good idea.
| import { useState, MouseEvent, KeyboardEvent } from 'react'; | ||
|
|
||
| type Props = { | ||
| children: JSX.Element; |
There was a problem hiding this comment.
nit: React.ReactNode instead of JSX
|
Function declarations: |
|
|
||
| type Props = { | ||
| children: JSX.Element; | ||
| summary: JSX.Element; |
There was a problem hiding this comment.
nit: React.ReactNode instead of JSX
| `; | ||
|
|
||
| export const ExpandableNode = ({ children, summary, detailsRef, depth = 0 }: Props) => { | ||
| const [isOpen, setOpen] = useState(true); |
There was a problem hiding this comment.
I don't see that it makes that much difference, but okay.
| export const ExpandableNode = ({ children, summary, detailsRef, depth = 0 }: Props) => { | ||
| const [isOpen, setOpen] = useState(true); | ||
|
|
||
| function handleClick(ev: MouseEvent) { |
There was a problem hiding this comment.
isn't the event type: React.MouseEvent<HTMLElement, MouseEvent>
There was a problem hiding this comment.
Not according to TypeScript. That's a standard HTML
tag, and following the chain of type declarations that VSCode gives me leads to this line in the TS lib:
onclick: ((this: GlobalEventHandlers, ev: MouseEvent) => any) | null;
There was a problem hiding this comment.
In the context of react, these are SyntheticEvent. However, as long as the type check is happy I'm fine.
| @@ -201,6 +203,11 @@ const DesignPage: React.FC<RouteComponentProps<{ dialogId: string; projectId: st | |||
| useEffect(() => { | |||
| if (location && props.dialogId && props.projectId) { | |||
| const { dialogId, projectId } = props; | |||
|
|
|||
| // TODO: swap to the commented-out block once we're working on skills for real | |||
There was a problem hiding this comment.
Please add a task id (github issue id) to finish this todo later on
There was a problem hiding this comment.
Good idea. Will do.
| @@ -455,7 +462,7 @@ const DesignPage: React.FC<RouteComponentProps<{ dialogId: string; projectId: st | |||
| function handleBreadcrumbItemClick(_event, item) { | |||
| if (item) { | |||
| const { dialogId, selected, focused, index } = item; | |||
| selectAndFocus(projectId, dialogId, selected, focused, clearBreadcrumb(breadcrumb, index)); | |||
| selectAndFocus(projectId, null, dialogId, selected, focused, clearBreadcrumb(breadcrumb, index)); | |||
There was a problem hiding this comment.
nit: it seems like this function args list is growing, how about changing it to accept an object so we know what is what by looking at it.
There was a problem hiding this comment.
That's a good idea, but refactoring all of these should be its own PR later (which I don't being assigned to).
| isRemote: boolean; | ||
| }; | ||
|
|
||
| type IProjectTreeProps = { |
There was a problem hiding this comment.
nit: Interfaces usually start with I (capital i), types are just PascalCase
There was a problem hiding this comment.
Fixed (my usual convention is to just name this Props, because we already know what component we're looking at the props of - it's the file name).
There was a problem hiding this comment.
sure thing, if it's not exported Props will do just fine.
|
|
||
| type IProjectTreeProps = { | ||
| onSelect?: (link: TreeLink) => void; | ||
| onAllSelected?: () => void; |
There was a problem hiding this comment.
nit: onSelectAll (to follow your other callbacks naming pattern)
There was a problem hiding this comment.
I almost named it that, but that reads to me like there's some kind of "select-all" action - as in "select everything in the tree somehow" when what I really mean is that the literal link named "All" got selected.
There was a problem hiding this comment.
I think I'll call this onSelectAllLink. That's different enough from "select all" that I think it reads better as "select the 'all' link".
| groupRef.current?.toggleCollapseAll(true); | ||
| props.onToggleCollapse?.(props.group!); | ||
| onSelect(props.group!.key); | ||
| const botHasErrors = (bot: BotInProject) => { |
There was a problem hiding this comment.
If you want to keep it here, please use useMemo
| label: formatMessage('Remove this dialog'), | ||
| icon: 'Delete', | ||
| action: (link) => { | ||
| onDeleteDialog(link.dialogName ?? ''); |
There was a problem hiding this comment.
what does it mean to delete a dialog with blank name?
There was a problem hiding this comment.
That's only there to make type-checking happy; this is inside the renderDialogHeader function, so we know that link.dialogName is never going to be undefined here, but TS doesn't necessarily know that.
| }, | ||
| }, | ||
| ]} | ||
| shiftOut={showTriggers ? 0 : 28} |
There was a problem hiding this comment.
nit: Please keep the number in a variable with a descriptive name, otherwise it seems like a magic number
There was a problem hiding this comment.
Good call. Will fix this.
…t#4361) * Update en-US.json * bring in stuff from the draft branch * make deletion work * add error/warning icons * read notification map for state * fix type-checking and start on unit tests * add sampleDialog and fix more tests * add showAll * rename to onAllSelected because it's a callback * update unit tests * fix onSelect handling in ProjectTree * Update qna.test.tsx * Update design.test.tsx * add unit test * fixes from PR comments Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Description
This swaps the Design View project tree to the new design, hopefully keeping all functionality intact while clearing space for the new form dialogs, added menu commands, and multi-skill bot handling.
Task Item
refs #3956
Screenshots