OCV first pass #10355

srietkerk · 2025-01-28T00:01:13Z

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

I likely should have split this into a separate PR, but I added a flag to the CLI to use https rather than always using http. This was needed because the OCV iframe only shows up for domains in an allow list, and the local development options they have for that allow list use https.
I added the 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

Programmatically changing the theme of the feedback for high contrast and so we can have different colors based on the target we're serving from
Use the "kind" flag that I've added to the feedback object to change the feedback config. I'm planning on following a similar pattern that the share modal uses with ShareInfo.
Get rating feedback to show upon finishing tutorials

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:

…erk/ocv-iframe

riknoll

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/Feedback.tsx

riknoll · 2025-01-28T00:55:34Z

react-common/components/controls/Feedback/FeedbackEventListener.ts

+ * @param {any} [callbacks]: an object of functions that can be called when certain events happen in the feedback modal.
+ *  Needs to be passed in because the callbacks will depend on what the parent wants to react to.
+ */
+export const initFeedbackEventListener = (feedbackConfig: any, frameId: string, callbacks?: any) => {


don't use any. same for the variables declared above.

react-common/components/controls/Feedback/FeedbackEventListener.ts

react-common/components/controls/Feedback/configs.ts

react-common/styles/controls/Feedback.less

riknoll · 2025-01-28T01:12:04Z

webapp/src/container.tsx

@@ -349,7 +355,7 @@ export class SettingsMenu extends data.Component<SettingsMenuProps, SettingsMenu
            {
                // we always need a way to clear local storage, regardless if signed in or not
            }
-            {targetTheme.feedbackUrl ? <a className="ui item" href={targetTheme.feedbackUrl} role="menuitem" title={lf("Give Feedback")} target="_blank" rel="noopener noreferrer" >{lf("Give Feedback")}</a> : undefined}
+            {targetTheme.giveFeedback ? <sui.Item role="menuitem" text={lf("🙂 Give Feedback")} onClick={this.showFeedbackDialog} /> : undefined}


this icon shouldn't be included in the lf string, otherwise a translator might move it so it's not left aligned like the rest of the menu items. same for the home menu item below

abchatra · 2025-01-28T17:04:16Z

Nice work

Co-authored-by: Richard Knoll <[email protected]>

thsparks · 2025-01-28T19:14:38Z

react-common/components/controls/Feedback/configs.ts

@@ -0,0 +1,57 @@
+export const appId = 50315;
+export const feedbackFrameUrl = 'https://admin-ignite.microsoft.com';


Are feedbackFrameUrl and appId things we should store in targetconfig?

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.

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.

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.

Somewhat related question: is the OCV/feedback feature only enabled for 1st party targets?

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.

Were you able to move these hard-coded config values to pxtarget.json?

thsparks · 2025-01-28T19:25:41Z

webapp/src/container.tsx

@@ -349,7 +355,7 @@ export class SettingsMenu extends data.Component<SettingsMenuProps, SettingsMenu
            {
                // we always need a way to clear local storage, regardless if signed in or not
            }
-            {targetTheme.feedbackUrl ? <a className="ui item" href={targetTheme.feedbackUrl} role="menuitem" title={lf("Give Feedback")} target="_blank" rel="noopener noreferrer" >{lf("Give Feedback")}</a> : undefined}
+            {targetTheme.giveFeedback ? <sui.Item role="menuitem" text={lf("🙂 Give Feedback")} onClick={this.showFeedbackDialog} /> : undefined}


IIRC, you mentioned in stand-up that the smiley face icon is jarring. I'm guessing that's why it's in the string here instead of the icon property on the Item. Per Richard's comment, we probably want to use the icon property. Did you consider using a speech bubble icon or bullhorn icon instead?

Ooh, I hadn't considered that! I was trying to align as close as possible to Microsoft's icon patterns, but I like the idea of the comment bubble. I'll see how that looks!

Alright, here's the different options, let me know what you think.

With emoji

With comment bubble

Hmm, personally I would vote for the comment icon (bonus points if there's a filled-in version to match the other icons). I feel like the color in the emoji contrasts strangely with what's already there.

react-common/components/controls/Feedback/configs.ts

eanders-ms · 2025-02-04T18:57:51Z

localtypings/pxtarget.d.ts

@@ -524,6 +524,7 @@ declare namespace pxt {
        timeMachineDiffInterval?: number; // An interval in milliseconds at which to take diffs to store in project history. Defaults to 5 minutes
        timeMachineSnapshotInterval?: number; // An interval in milliseconds at which to take full project snapshots in project history. Defaults to 15 minutes
        adjustBlockContrast?: boolean; // If set to true, all block colors will automatically be adjusted to have a contrast ratio of 4.5 with text
+        giveFeedback?: boolean; // Show the give feedback button in the settings menu


Nit: naming here could be clearer/more self-documenting. Some alternatives that come to mind: showFeedbackButton, feedbackButtonEnabled.

eanders-ms · 2025-02-04T18:58:43Z

localtypings/pxteditor.d.ts

@@ -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;


What does this field mean? Can it be better named? Or add a comment after it?

eanders-ms · 2025-02-04T19:04:13Z

react-common/components/controls/Feedback/configs.ts

@@ -0,0 +1,57 @@
+export const appId = 50315;
+export const feedbackFrameUrl = 'https://admin-ignite.microsoft.com';


Were you able to move these hard-coded config values to pxtarget.json?

eanders-ms · 2025-02-04T19:08:09Z

react-common/components/controls/Feedback/FeedbackEventListener.ts

+            baseTheme: currentTheme,
+        },
+    }
+    const iFrameElement = document.getElementById(FEEDBACK_FRAME_ID) as HTMLIFrameElement


Code duplication here. Move these two lines to a helper function. In that function, check for missing iFrameElement, as document.getElementById can return undefined.

eanders-ms · 2025-02-04T19:10:40Z

react-common/components/controls/Feedback/FeedbackEventListener.ts

+// TODO
+// haven't implemented yet with events, but this will be needed in order to update to high contrast
+// general changes need to be made as well use the correct theme. the windows ones were just the defaults.
+const sendUpdateTheme = () => {


Do you need to update themeOptions somewhere in this method, to keep it in sync?

eanders-ms · 2025-02-04T19:11:32Z

react-common/components/controls/Feedback/FeedbackEventListener.ts

+                break
+            case 'InAppFeedbackDismissWithResult': //Invoked when feedback is dismissed - the big important one for us to be able to close the feedback modal
+                pxt.debug('InAppFeedbackDismissWithResult: ', payload.EventArgs);
+                if (feedbackCallbacks.onDismiss) {


Suggested change

if (feedbackCallbacks.onDismiss) {

if (feedbackCallbacks?.onDismiss) {

Or even better: feedbackCallbacks?.onDismiss?.()

eanders-ms · 2025-02-04T19:14:18Z

react-common/components/controls/Feedback/FeedbackEventListener.ts

+        event: 'InAppFeedbackInitOptions',
+        data: initfeedbackOptions,
+    }
+    response = JSON.parse(JSON.stringify(response))


Is there a strong reason for deep copy here?

srietkerk added 15 commits December 12, 2024 11:38

plumbing for showing the feedback iframe

d31eca8

add https flag to the cli to use https when debugging ocv iframe

f55fca2

Merge branch 'master' of https://github.com/microsoft/pxt into srietk…

c705f96

…erk/ocv-iframe

more config, removed functions, changed height and width of iframe

87236db

using different modal for the feedback

0593552

added feedback menu item to editor menu

5259143

got rid of iframe border

9354dc2

some small changes and styling

a764d25

added comments

6446b2a

move app id to configs

2120588

fix onclose reference

4da2fdc

moved frame url to configs

05d4be7

remove tutorial completion feedback for now

816c662

use emoji instead of sui icon

8336f42

merged master

bab52a2

srietkerk requested a review from a team January 28, 2025 00:01

riknoll requested changes Jan 28, 2025

View reviewed changes

srietkerk and others added 7 commits January 28, 2025 10:12

update iframeelement variable in event listener

f905236

Co-authored-by: Richard Knoll <[email protected]>

add space to less file

4a4eb79

Co-authored-by: Richard Knoll <[email protected]>

another iframe element var change

6c86476

Co-authored-by: Richard Knoll <[email protected]>

moved height, width to less file, fixed some styling

cd43ef9

remove redundant jsdoc types

be206b5

use pxt log functions, fix spacing in the event listener

2bb7a05

move emoji out of lf

b209592

thsparks reviewed Jan 28, 2025

View reviewed changes

react-common/components/controls/Feedback/configs.ts Show resolved Hide resolved

srietkerk added 3 commits January 28, 2025 13:30

use lf for the displayed strings for the rating

9438c18

types added

158939c

move types to localtypings and wrap them in a namespace

c714b42

srietkerk added 2 commits January 30, 2025 15:21

change to using the comment bubble icon

728ebd8

added types

69ec94d

eanders-ms reviewed Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCV first pass #10355

OCV first pass #10355

srietkerk commented Jan 28, 2025

riknoll left a comment

riknoll Jan 28, 2025

riknoll Jan 28, 2025

abchatra commented Jan 28, 2025

thsparks Jan 28, 2025 •

edited

Loading

srietkerk Jan 28, 2025

thsparks Jan 29, 2025

eanders-ms Jan 29, 2025

eanders-ms Jan 29, 2025

srietkerk Jan 29, 2025

eanders-ms Feb 4, 2025

thsparks Jan 28, 2025

srietkerk Jan 28, 2025

srietkerk Jan 28, 2025

thsparks Jan 29, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025

eanders-ms Feb 4, 2025 •

edited

Loading

		@@ -0,0 +1,57 @@
		export const appId = 50315;
		export const feedbackFrameUrl = 'https://admin-ignite.microsoft.com';

	if (feedbackCallbacks.onDismiss) {
	if (feedbackCallbacks?.onDismiss) {

OCV first pass #10355

Are you sure you want to change the base?

OCV first pass #10355

Conversation

srietkerk commented Jan 28, 2025

riknoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abchatra commented Jan 28, 2025

thsparks Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

thsparks Jan 28, 2025 •

edited

Loading

eanders-ms Feb 4, 2025 •

edited

Loading