Skip to content

List: Add a _notifyPageChanges function #3990

Merged
cschleiden merged 4 commits intomicrosoft:masterfrom
abettadapur:alebet/onPageAdded
Feb 16, 2018
Merged

List: Add a _notifyPageChanges function #3990
cschleiden merged 4 commits intomicrosoft:masterfrom
abettadapur:alebet/onPageAdded

Conversation

@abettadapur
Copy link
Copy Markdown
Collaborator

@abettadapur abettadapur commented Feb 15, 2018

Pull request checklist

Description of changes

If the getPageHeight property is provided to List, the onPageAdded and onPageRemoved callbacks are never called.

This is because these callbacks are called within the updatePageMeasurements function, which does not need to be called if getPageHeight is provided.

The fix is to separate this logic into its own function, _notifyPageChanges, which is called in the _updatePages function

@cschleiden
Copy link
Copy Markdown
Contributor

Can you add a change file?

/**
* Notify consumers that the rendered pages have changed
* @param oldPages The old pages
* @param newPages The new pages
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add props?

[index: number]: IPage;
} = {};

for (let i = 0; i < oldPages.length; i++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for (const page of oldPages) {?

}
}

for (let i = 0; i < newPages.length; i++) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for (const page of newPages) {?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see, just moved the code. Might be fine then.

@cschleiden cschleiden merged commit 8b23510 into microsoft:master Feb 16, 2018
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Feb 18, 2018
* master: (71 commits)
  Applying package updates.
  Delete initials_2018-02-07-13-49.json
  Delete initials_2018-02-07-13-49.json
  Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json
  Delete initials_2018-02-07-13-49.json
  Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json
  Delete initials_2018-02-07-13-49.json
  Update .npmrc
  Cleaning Up Console Log in CommandBar Test (microsoft#4011)
  Convert Overlay to mergeStyles (microsoft#3978)
  ScrollablePane SCSS to MergeStyles Part 1: File Structure (microsoft#4008)
  [ContextualMenu] Fixes useTargetWidth property (microsoft#3943)
  DetailsList: Consider groups when setting aria-rowcount
  List: Add a _notifyPageChanges function  (microsoft#3990)
  Applying package updates.
  Migrating Coachmark to main Package, Added a beak component and updated experiment PositioningContainer. (microsoft#3919)
  Update package.json
  Added enum for triggering menu with arrow keys and bool to allow it or not (microsoft#3950)
  BaseExtendedPicker: Hook up onPaste (microsoft#3885)
  FocusUtil: fix getPreviousElement to include previous sibling elements correctly (microsoft#3928)
  ...
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List: onPageAdded/Removed is not called if getPageHeight is provided as a prop

3 participants