Skip to content
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

[Word] Add snippets for APIs re pages, panes, windows #974

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ElizabethSamuel-MSFT
Copy link
Contributor

Will do object mapping later

order: 4
id: word-ranges-get-pages
name: 'Work with pages, panes, and windows'
description: 'Shows how to work with pages, panes, and windows APIs.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we're working with APIs is already implied since we're in Script Lab.

Suggested change
description: 'Shows how to work with pages, panes, and windows APIs.'
description: 'Shows how to work with pages, panes, and windows.'


async function getPagesOfThirdParagraph() {
await Word.run(async (context) => {
// Gets the pages that the third paragraph is found on.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dangling "on" seemed off, but I think this is the most concise way to articulate the relationships.

Suggested change
// Gets the pages that the third paragraph is found on.
// Gets the pages that contain the third paragraph.


const pages: Word.PageCollection = rangeOfParagraph.pages;
pages.load();
await context.sync();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need three roundtrips to the document before even getting to the API in question? If so, that seems like a product flaw.

// Gets the pages enclosing the viewport.

// Get the active window.
let activeWindow = context.document.activeWindow;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We switched from let to const partway through the snippet. Let's use const here and elsewhere.

Suggested change
let activeWindow = context.document.activeWindow;
const activeWindow = context.document.activeWindow;

Comment on lines +85 to +86
await context.sync();
// Get pages enclosing the viewport.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find adding a blank line after syncs helps with readability.

Suggested change
await context.sync();
// Get pages enclosing the viewport.
await context.sync();
// Get pages enclosing the viewport.

for (let i = 0; i < pages.items.length; i++) {
let page = pages.items[i];
page.load();
await context.sync();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid doing load/sync in a loop. It's a bad pattern. This article has some tips to avoid that: https://learn.microsoft.com/en-us/office/dev/add-ins/concepts/correlated-objects-pattern

template:
content: |-
<section class="ms-Fabric ms-font-m">
This sample demonstrates how to work with the pages, panes, and windows APIs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This sample demonstrates how to work with the pages, panes, and windows APIs.
This sample demonstrates how to work with pages, panes, and windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants