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.
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
Changes from 15 commits
d31eca8
f55fca2
c705f96
87236db
0593552
5259143
9354dc2
a764d25
6446b2a
2120588
4da2fdc
05d4be7
816c662
8336f42
bab52a2
f905236
4a4eb79
6c86476
cd43ef9
be206b5
2bb7a05
b209592
9438c18
158939c
c714b42
728ebd8
69ec94d
9dc5367
3e8c6ba
83b45f8
980020a
3b6a3ff
943c493
36c823c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?
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?