Skip to content

In Panel component, allow user to override all focus zone props.#3568

Merged
joschect merged 6 commits into
microsoft:masterfrom
SpencerLynn:panel-focus-zone-props
Jan 23, 2018
Merged

In Panel component, allow user to override all focus zone props.#3568
joschect merged 6 commits into
microsoft:masterfrom
SpencerLynn:panel-focus-zone-props

Conversation

@SpencerLynn

@SpencerLynn SpencerLynn commented Dec 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

In Panel component, allow consumers to override all FocusTrapZone props. This change removes the few props that were only used for the FocusTrapZone. I'm not sure what the breaking change policy is, but I'm happy to modify this to meet the qualifications needed.

@SpencerLynn

SpencerLynn commented Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

@dzearing Can you help provide some input on this, as it removes props as values. Is there a method of deprecating them instead, perhaps? Then remove them in the future?

@SpencerLynn

SpencerLynn commented Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

I have marked the props previously used only to pass directly to the FocusTrapZone has deprecated and spread in the new prop I added to override the old ones if they are still used.

@SpencerLynn

SpencerLynn commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

@dzearing or anyone else have any update or thoughts on this PR?

@joschect

joschect commented Jan 12, 2018

Copy link
Copy Markdown
Contributor

Hey @SpencerLynn one more suggestion, and then I think this pr is ready to go. You should add this._warnDeprecations({ 'propertyNameThatsDeprecated': 'ReplacementProperty' }); in the constructor.
So for this change it would be something like this._warnDeprecations({'firstFocusableSelector':'focuszoneprops', (and so on)})

@SpencerLynn

SpencerLynn commented Jan 13, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @joschect ! Good to know about that function! Done.

@SpencerLynn

SpencerLynn commented Jan 20, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @joschect for the review! Is this ready to be merged? Do we need any one else to review these changes?

@joschect

joschect commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

@SpencerLynn I think that this can probably be merged.

@SpencerLynn

SpencerLynn commented Jan 23, 2018

Copy link
Copy Markdown
Contributor Author

Ok great @joschect. I do not have merge privileges.

Do we need @dzearing or @eliblock12? Or can you do it?

@joschect joschect merged commit 2a57fb1 into microsoft:master Jan 23, 2018
@SpencerLynn SpencerLynn deleted the panel-focus-zone-props branch January 24, 2018 02:27
@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.

Reviewers

@dzearing dzearing Awaiting requested review from dzearing
@eliblock12 eliblock12 Awaiting requested review from eliblock12
1 more reviewer
@joschect joschect joschect approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SpencerLynn @joschect