-
Notifications
You must be signed in to change notification settings - Fork 750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ISSUE#1397] Fix file explorer opening when clicking on HeroCard button #1631
Conversation
@@ -52,6 +52,8 @@ export default function interceptHyperlink() { | |||
|
|||
// Monkey patch window.open | |||
window.open = (url: string): any => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing wrong with this approach however, I'm confident this logic should be reviewed for incorporation into WebChat at this location. It seems like an enhancement that could have value there.
The window.open ponyfill in WebChat landed here. |
I'm seeing that just now. It looks like we want to implement the @denscollo - From the information @cwhitten provided, you should work towards the implementation listed in this example as it represents a more idiomatic approach to this solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this example to solve this defect. Feel free to reach out if you need help.
@justinwilaby @cwhitten thanks for the feedback! We'll be working by using the example and let you know as soon as we have the new solution. |
…o-card-open-file-explorer
…/Microsoft/BotFramework-Emulator into fix/hero-card-open-file-explorer
@denscollo is this ready for review? At the moment we're seeing an extremely small diff. |
@denscollo I believe what Justin meant by using that example, is that we want to implement the So it would look something like: chat.tsx
|
Thank you for providing this clarification @tonyanziano |
@justinwilaby @tonyanziano Thanks for the clarification! This is still a WIP, we are working on it. We are reviewing the changes on our fork and then we'll push them. |
@justinwilaby @tonyanziano the PR is now ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to have to do this again. It's our fault for not being specific on the original github issue. I'm requesting changes to remove confirm
and use a custom dialog using the DialogService
. This will maintain consistency across the app and prevent possible side-effects from a blocking operation.
Also, since this is new functionality, unit tests need to accompany this PR.
case 'playVideo': | ||
case 'showImage': | ||
case 'openUrl': | ||
if (confirm(`Do you want to open this URL?\n\n${value}`)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically favor custom dialogs over native since UX continuity is better across the app. Also, confirm
is thread-blocking and may have side-effects if the main process sends IPC messages during the display of this confirmation modal.
I realize this is more work to create a custom prompt using the DialogService
but I believe it's the right thing to do. Please feel free to reach out to me if you need guidance on implementing this.
@justinwilaby Ok, we'll be working on this approach and let you know if we have doubts. Thanks! |
f156821
to
85443fd
Compare
@justinwilaby we finished applying the feedback, so it's ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to pull the branch down and test it out locally. I had some minor comments.
packages/app/client/src/ui/dialogs/openUrlDialog/openUrlDialog.spec.tsx
Outdated
Show resolved
Hide resolved
packages/app/client/src/ui/dialogs/openUrlDialog/openUrlDialogContainer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now!
Fixes #1397
Description
Added validation to check the value of the URL before using it because if it's empty, the file explorer will open by default.
Specific changes
Testing
We used the user hero card to test the fix.
![image](https://user-images.githubusercontent.com/37461749/59378750-6e96d680-8d2b-11e9-8462-b2731ae7fc75.png)