-
Notifications
You must be signed in to change notification settings - Fork 128
[Excel] Map existing snippets to ref docs #1000
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
Conversation
| context.application.calculate(Excel.CalculationType.recalculate); | ||
| await context.sync(); | ||
| }); | ||
| Excel.CardLayoutSection:type: |
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'm worried this snippet is so large, it'll be hard to find the relevant section. Is all this context necessary, or it is an artifact of the way the snippet mapping works?
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.
Good point. I updated the PR and mapped CardLayoutSection to a different, shorter snippet. I also mapped EntityPropertyType and EntityViewLayouts (others in this PR) to the shorter snippet.
AlexJerabek
left a comment
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 @alison-mk! I think these are good snippets to use. My one thought is that it's not clear where the interface is used in some of these samples. We often make the type explicit (e.g. const entity: Excel.EntityCellValue = ...) so that people understand how the code sample links to the reference page, without needing to understand the object model (e.g. the DV sample requires you know rankingRange.dataValidation.errorAlert is of type DataValidationErrorAlert). Consider adding explicit typing or comments to clear this up.
| https://raw.githubusercontent.com/OfficeDev/office-js-snippets/prod/samples/excel/20-data-types/data-types-entity-attribution.yaml | ||
|
|
||
|
|
||
| function makeProductEntity(productID: number, productName: string, product?: |
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.
Where is the actual CardLayoutSection in this sample? It's not obvious (to me) from skimming 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.
Great feedback! I added the explicit types as inline comments. Please take a look and let me know what you think.
AlexJerabek
left a comment
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!
No description provided.