Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@a-b-r-o-w-n
Copy link
Contributor

Description

  • Includes PVA topics in the select dialog dropdown
  • Instead of navigating in app, opens the topic in PVA
  • Shows display name of dialog instead of dialog id in visual node

Task Item

refs #6047

Screenshots

  • TODO

There has been a change in how content diff is calculated that no longer requires this.
@a-b-r-o-w-n a-b-r-o-w-n marked this pull request as draft April 6, 2021 20:51
export const DialogRef: WidgetComponent<DialogRefCardProps> = ({ id, onEvent, dialog }) => {
export const DialogRef: WidgetComponent<DialogRefCardProps> = ({ id, onEvent, dialog, data }) => {
const { ElementWrapper } = useContext(RendererContext);
const { dialogs, topics } = useContext(NodeRendererContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yeze322 Can you take a look at this and let me know your thoughts?

I wanted to do 2 things:

  1. Use the dialog's display name instead of its id
  2. Differentiate from dialogs and topics

This is your domain so I want to make sure that this approach is fine, or work with you to do something better.

@cwhitten cwhitten added this to the R13 milestone Apr 7, 2021
@cwhitten cwhitten marked this pull request as ready for review April 8, 2021 19:26
@cwhitten cwhitten requested a review from benbrown as a code owner April 8, 2021 19:26
const isRoot = file.relativePath.includes('/') === false; // root dialog should be in root path
const dialog: DialogInfo = {
isRoot,
isTopic: file.relativePath.startsWith('topics/'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the answer is yes, but just confirming: We are fine with not indexing the topic if the user decides to muck around with the folder structure right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should engage the PVA team on that. My assumption is that the folder structure is necessary when publishing back to PVA, but I don't know for sure.

Copy link
Contributor

@tonyanziano tonyanziano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but going to wait until Ze responds to your comment before approving.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 51.157% when pulling 1d1fd54 on abrown/pva-topics into da88728 on main.

@a-b-r-o-w-n a-b-r-o-w-n merged commit 06c0d6e into main Apr 14, 2021
@a-b-r-o-w-n a-b-r-o-w-n deleted the abrown/pva-topics branch April 14, 2021 22:50
@cwhitten cwhitten mentioned this pull request May 20, 2021
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
…rosoft#6691)

* add includeTopics query when downloading bot content

* include topics when loading bots

* do not show topics in the main dialog navigation

* move icons into shared

* add more icons

* add isTopic to DialogInfo

* expose topics through shell api

* show topics in begin dialog action

* link to PVA topic

* include pva topics by default for electron task

* update query string to be boolean value

* do not include etag when publishing with topics

There has been a change in how content diff is calculated that no longer requires this.

* make icons in dropdowns blue

* revert If-Match header change

Bug was fixed in PVA

* default to using oneauth in electron launch task

* silence console output for tests in CI

* fix select dialog test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants