-
Notifications
You must be signed in to change notification settings - Fork 593
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
OCV first pass #10355
OCV first pass #10355
Conversation
…erk/ocv-iframe
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.
would be nice to have typings for these configs! they don't have to be exhaustive, but should include the fields you're referencing. we can expand them later as we add more code
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
Nice work |
Co-authored-by: Richard Knoll <[email protected]>
Co-authored-by: Richard Knoll <[email protected]>
Co-authored-by: Richard Knoll <[email protected]>
@@ -0,0 +1,57 @@ | |||
export const appId = 50315; | |||
export const feedbackFrameUrl = 'https://admin-ignite.microsoft.com'; |
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.
Are feedbackFrameUrl and appId things we should store in targetconfig?
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.
This was definitely something I was considering, but these are going to be the same for each of our targets, so it feels redundant to put these in the target config of each target.
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.
I suppose that's true. Ideally, I think we'd want to put this somewhere that wouldn't require a release to change (i.e. what happened with our CDN) but I'm not sure if we have a good option for that without going through our backend.
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.
I agree this url should go in pxtarget.json, even if it is the same for each target. This is what we do for privacyUrl
, termsOfUseUrl
, etc.
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.
Somewhat related question: is the OCV/feedback feature only enabled for 1st party targets?
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.
Yes, we should only be showing feedback in our self-hosted targets. There is more work to really verify this, which will be my next priority, but for now, it's just something that gets set in the apptheme as something to include in the target.
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.
Were you able to move these hard-coded config values to pxtarget.json?
localtypings/pxteditor.d.ts
Outdated
@@ -805,6 +805,7 @@ declare namespace pxt.editor { | |||
extensionsVisible?: boolean; | |||
isMultiplayerGame?: boolean; // Arcade: Does the current project contain multiplayer blocks? | |||
onboarding?: pxt.tour.BubbleStep[]; | |||
feedback?: boolean; |
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.
What does this field mean? Can it be better named? Or add a comment after it?
@@ -0,0 +1,57 @@ | |||
export const appId = 50315; | |||
export const feedbackFrameUrl = 'https://admin-ignite.microsoft.com'; |
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.
Were you able to move these hard-coded config values to pxtarget.json?
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.ts
Outdated
Show resolved
Hide resolved
react-common/components/controls/Feedback/FeedbackEventListener.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 good!
This is the work to plug in the One Customer Voice iframe into our products. Right now, it's only getting triggered from our settings menus, but we can pop this modal up in any context.
Things of note in this PR
giveFeedback
option to the appTheme object as a starting point for only showing the feedback menu item for our targets.FeedbackListener.ts
is where the logic to get OCV to work lives. There is a lot of generic code in there and console logs that I figure we'll want to update to something more clean/helpful, but I kept it in for now for simplicity. A note about the event listener: the ones that I kept in are the ones I figured would be useful for us. There were some others, but I removed them because for our use case, they wouldn't really apply. If you would like to see the other events, I would be happy to showcase them.To Dos
Testing
Unfortunately, I cannot make an upload target that will be useful for these changes because of the allow list that I talked about before. I've pinged the OCV teams a couple of times to get the makecode domain in their allow list but I'm not sure of the progress. If I made an upload target, you would be able to see the "Give Feedback" option in the setting menu, but clicking on it will give you a modal with a broken iframe. If you want to actually try out the forms, you can test locally with https. I'm going to write up a doc on how to test OCV locally.
In the meantime, here's how it looks:
![image](https://private-user-images.githubusercontent.com/49178322/407132537-70e67b67-fdab-4a6f-9059-43c85621c3d1.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwODQ5NzIsIm5iZiI6MTczOTA4NDY3MiwicGF0aCI6Ii80OTE3ODMyMi80MDcxMzI1MzctNzBlNjdiNjctZmRhYi00YTZmLTkwNTktNDNjODU2MjFjM2QxLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA5VDA3MDQzMlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWE0Mzc2NjFiMjk5Y2JiODAwYWQ2M2UwYWMzNjg5NTIyZTUwNGYyNDA0ODQ1YjljOTM5ZTFhMTg4ZGY2MzM4MzUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.0gLUZ96mdX0yqr1PN_DgWSaxRHbRjS5g6YcUbbLpofM)