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
AppNotificationContent Builder Core Features - part II #2760
AppNotificationContent Builder Core Features - part II #2760
Changes from all commits
7119200
4765ea2
30a4376
40bd764
8668d3a
1545006
e470027
422eae8
f55aad3
aa7ed04
e0b2847
15b477c
903956a
bbe8a26
f5a745f
435a152
238684d
5423935
d427450
5dc491a
2b24c69
33a4b1b
5bd1f9e
41a699e
5ea7ddd
019077a
48fcdc7
f45fb7a
f81258e
8851cae
224877f
861014d
a934566
19d0cd4
195492a
9c6bb22
8f51b89
4b59d7f
f8ed6ca
49c02bb
3b3b08c
c86c655
604a24f
203b318
f8e8b30
83f3e0e
c53c4dd
69e458f
162cd55
493f0a6
eeccf81
6624283
0b994b3
b3de669
5dd8f22
d52a87c
01ea539
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.
Is ID restricted to never allow standard entities that need to be escaped?
standard entities are the gang of 5 -- greater-than, less-than, apostraphe, quote, ampersand
same comment applies in general - any data the Builder accepts that could contain these escape their content appropriately, e.g.
Bogus but representative example
builder.AddButton("Ben & Jerry's")
yields what?<Button Name="Ben & Jerry's">
==> not well-formed XML<Button Name="Ben & Jerry's">
==> well-formed XMLThere 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 like it doesn't like " & <, but also it doesn't mind empty string. I am introducing an escape character map in my PR :#2805 @loneursid. We can add these to this map when my PR merges.
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 are using percent encoding for our special characters: https://developer.mozilla.org/en-US/docs/Glossary/percent-encoding
Go ahead and merge this PR when ready, I'll raise another PR regarding this issue since it would require more of a sweeping change in AppNotificationBuilder.
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.
Yeah, I'll do that. I'll merge this as is, since it's approved and the build is passing and raise a new PR to address nits from this and other PRs in the series
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.
https://stackoverflow.com/questions/1091945/what-characters-do-i-need-to-escape-in-xml-documents
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.
Addressing in: #2813
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.
Thanks @DrusTheAxe for catching this. It totally slipped past me.
Since we put anything that comes to us through the builder API into single quotes, technically, that the only character we need to escape, right? Or am I missing something?
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 have an opportunity here to do some extra validation that goes beyond what the xml schema prescribes. For example, we could ensure that a input-hint in a button elements, refers an existing texbox. That's probably something worth exploring in a future iteration.
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.
Captured here: https://microsoft.visualstudio.com/OS/_workitems/edit/40746502
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.
NIT:
return result.empty() ? ...
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.
Fixed in #2813
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 awesome to have a comment on how a final string would look like with the payload.
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 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've tried to cobble something together with an image, some text, a combobox, a textbox and two buttons and I'm not convainced this would be useful.
I think i't better/easier to lookup the schema online when working in the builder.
I'm also concerned that this kind of comment will get out of sync quickly and become a maintenance nightmare.
I could create a unittest that creates an AppNotification simillar to this example comment. At least we know that this would never get out of sync with the code.
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.
Examples in the spec and documentation are welcome (and encouraged).
Not so useful as a comment.
Maybe useful as a doc-comment, if it can be small and meaningful.
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.
NIT: GitHub dislikes files ending without a new line
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.
Done: #2813