Add links to docs in UI #842
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
September 11, 2018 12:14
compulim
reviewed
Sep 11, 2018
| AzureLoginSuccessDialogContainer, | ||
| AzureLoginFailedDialogContainer)); | ||
| AzureLoginPromptDialogContainer, | ||
| { ServiceTypes }, |
There was a problem hiding this comment.
I guess this line should be { serviceType: ServiceTypes.Luis }.
compulim
reviewed
Sep 11, 2018
| AzureLoginSuccessDialogContainer, | ||
| AzureLoginFailedDialogContainer)); | ||
| AzureLoginPromptDialogContainer, | ||
| { ServiceTypes }, |
compulim
reviewed
Sep 11, 2018
| AzureLoginSuccessDialogContainer, | ||
| AzureLoginFailedDialogContainer)); | ||
| AzureLoginPromptDialogContainer, | ||
| { ServiceTypes }, |
compulim
reviewed
Sep 11, 2018
compulim
reviewed
Sep 11, 2018
| function* launchConnectedServicePickList( | ||
| action: ConnectedServiceAction<ConnectedServicePickerPayload>, | ||
| availableServices: IConnectedService[], | ||
| serviceType: ServiceTypes, |
compulim
reviewed
Sep 11, 2018
| @@ -278,20 +286,21 @@ function openLuisDeepLink(luisService: ILuisService): Promise<any> { | |||
| regionPrefix = ''; | |||
| break; | |||
| } | |||
| const link = `https://www.${regionPrefix}luis.ai/applications/${appId}/versions/${version}/build`; | |||
| const link = `https://www.${ regionPrefix }luis.ai/applications/${ appId }/versions/${ version }/build`; | |||
There was a problem hiding this comment.
We should always build links with cautions. Do something like https://www.${ encodeURI( regionPrefix ) }luis.ai/....
compulim
reviewed
Sep 11, 2018
| return CommandServiceImpl.remoteCall(SharedConstants.Commands.Electron.OpenExternal, link); | ||
| } | ||
|
|
||
| function openQnaMakerDeepLink(service: IQnAService): Promise<any> { | ||
| const { kbId } = service; | ||
| const link = `https://qnamaker.ai/Edit/KnowledgeBase?kbid=${kbId}`; | ||
| const link = `https://qnamaker.ai/Edit/KnowledgeBase?kbid=${ kbId }`; |
There was a problem hiding this comment.
Use encodeURIComponent(kbId) here.
encodeURI is for path.
encodeURIComponent is for values inside query params.
compulim
reviewed
Sep 11, 2018
| return CommandServiceImpl.remoteCall(SharedConstants.Commands.Electron.OpenExternal, link); | ||
| } | ||
|
|
||
| function openAzureBotServiceDeepLink(service: IBotService): Promise<any> { | ||
| const { tenantId, subscriptionId, resourceGroup, id } = service; | ||
| const thankYouTsLint = `https://ms.portal.azure.com/#@${tenantId}/resource/subscriptions/${subscriptionId}`; | ||
| const link = `${thankYouTsLint}/resourceGroups/${resourceGroup}/providers/Microsoft.BotService/botServices/${id}`; | ||
| const thankYouTsLint = `https://ms.portal.azure.com/#@${ tenantId }/resource/subscriptions/${ subscriptionId }`; |
compulim
reviewed
Sep 11, 2018
| const thankYouTsLint = `https://ms.portal.azure.com/#@${tenantId}/resource/subscriptions/${subscriptionId}`; | ||
| const link = `${thankYouTsLint}/resourceGroups/${resourceGroup}/providers/Microsoft.BotService/botServices/${id}`; | ||
| const thankYouTsLint = `https://ms.portal.azure.com/#@${ tenantId }/resource/subscriptions/${ subscriptionId }`; | ||
| const link = |
compulim
reviewed
Sep 11, 2018
| @@ -13,16 +14,30 @@ export class AzureLoginPromptDialog extends Component<AzureLoginPromptDialogProp | |||
| <Dialog cancel={ this.props.cancel } title="Sign in with an Azure account"> | |||
| <p>Use your Azure account to sign in to all your Azure services, such as Azure Bot Service, Dispatch, | |||
| LUIS, ans QnA Maker. | |||
There was a problem hiding this comment.
(Optional) Try not to use as a space, it's different than a real space.
compulim
reviewed
Sep 11, 2018
| white-space: normal; | ||
| text-decoration: none; | ||
|
|
||
| &:hover { |
compulim
reviewed
Sep 11, 2018
| <TextField | ||
| className={ styles.smallInput } | ||
| inputClassName="bot-creation-input" | ||
| value={ endpoint.appPassword } | ||
| onChanged={ this.onChangeAppPw } | ||
| label={ 'MSA app password' } placeholder={ 'Optional' } type={ 'password' }/> | ||
| label={ 'MSA app password' } placeholder={ 'Optional' } type={ 'password' } /> |
There was a problem hiding this comment.
label="MSA app password"- Split it out into multiple lines
- (Optional) Sort the lines
compulim
reviewed
Sep 11, 2018
| className={ 'secret-checkbox' } | ||
| checked={ secretEnabled } | ||
| onChange={ this.onToggleSecret } | ||
| label={ 'Encrypt keys store in your bot configuration.' } id={ 'bot-secret-checkbox' } /> |
There was a problem hiding this comment.
label="Encrypt keys store in your bot configuration."- (Optional) Can we not to use
id? It pollute the environment
compulim
reviewed
Sep 11, 2018
| onChange={ this.onToggleSecret } | ||
| label={ 'Encrypt keys store in your bot configuration.' } id={ 'bot-secret-checkbox' } /> | ||
| | ||
| <a className={ styles.link } href="https://aka.ms/about-bot-file">Learn more.</a> |
compulim
reviewed
Sep 11, 2018
| <Dialog | ||
| cancel={ this.props.cancel } | ||
| title="Connect your bot to a QnA Maker knowledge base"> | ||
| <p>Sign in to your Azure account to select the QnA Maker knowledge |
There was a problem hiding this comment.
Indentation, or split it out like
<p>
Sign to your Azure ...
bases you'd like
<a href="...">Learn more ...</a>
</p>
compulim
reviewed
Sep 11, 2018
| <Dialog | ||
| cancel={ this.props.cancel } | ||
| title="Connect your bot to a Dispatch model"> | ||
| <p>Sign in to your Azure account to select the Dispatch model you'd like to associate with this bot. |
compulim
reviewed
Sep 11, 2018
| cancel: () => DialogService.hideDialog(0), | ||
| confirm: () => DialogService.hideDialog(1), | ||
| addLuisAppManually: () => DialogService.hideDialog(2) | ||
| addLuisAppManually: () => DialogService.hideDialog(2), | ||
| onAnchorClick: (url) => { |
There was a problem hiding this comment.
(Optional) No parenthesis for single argument for Lambda functions.
compulim
reviewed
Sep 11, 2018
| SecretPromptDialog: function mock() { | ||
| return undefined; | ||
| } | ||
| } |
compulim
reviewed
Sep 11, 2018
| @@ -23,7 +42,7 @@ describe('The GetStartedWithCSDialog component should', () => { | |||
| mockStore = createStore(combineReducers({ azureAuth })); | |||
| mockStore.dispatch(azureArmTokenDataChanged(mockArmToken)); | |||
| parent = mount(<Provider store={ mockStore }> | |||
There was a problem hiding this comment.
(Optional) Split this line into its own line.
parent = mount(
<Provider>
...
</Provider>
);
compulim
reviewed
Sep 11, 2018
| <a href="javascript:void(0);" onClick={ this.props.launchConnectedServiceEditor }> | ||
| connect to a LUIS app manually | ||
| </a> | ||
| if you know the app ID, version, and authoring key. |
compulim
reviewed
Sep 11, 2018
| } | ||
| private get dispatchContent(): JSX.Element { | ||
| const { showNoModelsFoundContent, authenticatedUser } = this.props; | ||
| if (!showNoModelsFoundContent) { |
compulim
reviewed
Sep 11, 2018
| private get dispatchContent(): JSX.Element { | ||
| const { showNoModelsFoundContent, authenticatedUser } = this.props; | ||
| if (!showNoModelsFoundContent) { | ||
| return ( |
compulim
reviewed
Sep 11, 2018
| if you know the app ID, version, and authoring key. | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
(Optional) Consider use else here for clarity. I think Uglify will remove it for you for shorter code anyway.
compulim
reviewed
Sep 11, 2018
| </p> | ||
| Alternatively, you can | ||
| <a href="javascript:void(0);" onClick={ this.props.launchConnectedServiceEditor }> | ||
| connect to a Dispatch app manually |
compulim
reviewed
Sep 11, 2018
| by entering this app ID and key. | ||
| </p> | ||
| <p> | ||
| <a href="javascript:void(0)" onClick={ this.onLearnMoreDispatch }>Learn more about Dispatch models</a> |
There was a problem hiding this comment.
I think we don't need the trailing .
compulim
reviewed
Sep 11, 2018
| const [, payload] = token.split('.'); | ||
| const pJson = JSON.parse(atob(payload)); | ||
| return {...ownProps, user: (pJson.upn || pJson.unique_name || pJson.name || pJson.email) }; | ||
| return { ...ownProps, user: (pJson.upn || pJson.unique_name || pJson.name || pJson.email) }; |
There was a problem hiding this comment.
Empty line before return and after const.
compulim
reviewed
Sep 11, 2018
| const [, payload] = token.split('.'); | ||
| const pJson = JSON.parse(atob(payload)); | ||
| return {...ownProps, user: (pJson.upn || pJson.unique_name || pJson.name || pJson.email) }; | ||
| return { ...ownProps, user: (pJson.upn || pJson.unique_name || pJson.name || pJson.email) }; |
There was a problem hiding this comment.
No need to use parenthesis here. user: pJson.upn || pJson.unique_name || pJson.name || pJson.email.
compulim
reviewed
Sep 11, 2018
packages/app/client/src/ui/dialogs/getStartedWithCSDialog/getStartedWithCSDialogContainer.ts
Show resolved
Hide resolved
compulim
reviewed
Sep 11, 2018
| close: () => void; | ||
| onAnchorClick: (url: string) => void; | ||
| } | ||
| export class PostMigrationDialog extends React.Component<PostMigrationDialogProps> { |
There was a problem hiding this comment.
New line after interface and before class.
compulim
reviewed
Sep 11, 2018
| <Dialog cancel={ this.onClose } className={ styles.postMigrationDialog } title="Migration complete!"> | ||
| <p>We’ve copied your bot endpoints from Emulator v3 and saved them as <strong>.bot files</strong>. | ||
| A <strong>.bot file</strong> stores metadata about different services your bot consumes | ||
| and enables you to edit these services directly from the Emulator v4. |
compulim
reviewed
Sep 11, 2018
| </p> | ||
| <p>You can move a bot to any location by right-clicking the bot’s name under My Bots. | ||
| <a href="">Learn more developing locally.</a> | ||
| <p>You can move a bot to any location by right-clicking the bot's name under My Bots. |
There was a problem hiding this comment.
- Snap
</p>back to the line - Do we need trailing
here?
compulim
reviewed
Sep 11, 2018
| import { PostMigrationDialog, PostMigrationDialogProps } from './postMigrationDialog'; | ||
| import { SharedConstants } from '@bfemulator/app-shared'; | ||
|
|
||
| const mapStateToProps = (ownProps: PostMigrationDialogProps) => { |
There was a problem hiding this comment.
(Optional) Consider:
const mapStateToProps = (ownProps: PostMigrationDialogProps) => ownProps;
compulim
reviewed
Sep 11, 2018
| onClick={onNewBotClick}>create a new bot configuration.</button> | ||
| <span>If you don’t have a bot configuration, | ||
| <button className={ styles.ctaLink } | ||
| onClick={ onNewBotClick }>create a new bot configuration.</button> |
compulim
reviewed
Sep 11, 2018
| <div className={styles.recentBotActionBar}> | ||
| <button data-index={index} onClick={this.onDeleteBotClick}></button> | ||
| <li className={ styles.recentBot } key={ bot.path }> | ||
| <button data-index={ index } onClick={ this.onBotClick } |
There was a problem hiding this comment.
Put one prop on its own line.
added 2 commits
September 11, 2018 15:29
tonyanziano
reviewed
Sep 12, 2018
There was a problem hiding this comment.
Made some minor suggestions. Otherwise looks good to me.
@justinwilaby Since this is a case where only markup was changed and no functionality changed, I think it's acceptable to overlook the minor decrease in code coverage. The drop was only caused due to an increase in markup. What are your thoughts?
...ges/app/client/src/ui/dialogs/connectLuisAppPromptDialog/connectLuisAppPromptDialog.spec.tsx
Outdated
Show resolved
Hide resolved
packages/app/client/src/ui/dialogs/connectLuisAppPromptDialog/connectLuisAppPromptDialog.tsx
Outdated
Show resolved
Hide resolved
This was referenced Nov 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #816