Skip to content

Add the ability to use custom props for the panel used to render options#4438

Merged
micahgodbolt merged 1 commit intomicrosoft:masterfrom
joeuy:jochiari/cutom-props
Apr 4, 2018
Merged

Add the ability to use custom props for the panel used to render options#4438
micahgodbolt merged 1 commit intomicrosoft:masterfrom
joeuy:jochiari/cutom-props

Conversation

@joeuy
Copy link
Copy Markdown
Contributor

@joeuy joeuy commented Apr 2, 2018

Add the ability to use custom props for the panel used to render options on small devices.
This change follows the precedent of calloutProps used for rendering options on other devices.

Pull request checklist

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

* Custom properties for ISelectableDroppableText's Panel used to render options on small devices.
*/
panelProps?: IPanelProps;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you explain why panelProps needs to be added to this types file?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The dropdown component renders a callout for a normal sized device, and a panel for small devices. Inside calloutProps, there's a field for specifying focustrapprops (and inside that, we can set the field forcefocusinsidetrap to be false). By setting these fields, we can click "outside" (ie, past the first page of items) and not have the focus forced to the top of the dropdown list. We need the ability to set these same props for the mobile implementation, since when the dropdown is rendered on mobile, it's a panel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand that panelProps only apply to Dropdown component (when rendered in small devices), and SelectableDroppableText.types.ts factors out common props to both Dropdown and ComboBox component. However, because SelectableDroppableText.types.ts already contains calloutProps I though it was confusing to devs to find panelProps on a different file. Moreover, eventually ComboBox could benefit of the same behavior dropdown has today for small devices. Let me know your thoughts, thanks!

(
<Panel
className={ css('ms-Dropdown-panel', styles.panel) }
className={ css('ms-Dropdown-panel', styles.panel, panelProps ? panelProps.className : undefined) }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

!!panelProps && panelProps.className instead of a ternary should work fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi,
I followed the style implied on line 378:
className={ css('ms-Dropdown-callout', styles.callout, calloutProps ? calloutProps.className : undefined) }

Functionally its the same thing, let me know if you want me to change both.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd say we're moving away from the ternaries unless the 3rd option is actually valuable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cool. I will update both then. What about the other comment? any other concerns before I update?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great to me. I approved

@joeuy joeuy force-pushed the jochiari/cutom-props branch from e1aa2ae to c20b178 Compare April 4, 2018 01:38
…ons on small devices.

This change follows the precedent of calloutProps used for rendering options on other devices.
@joeuy joeuy force-pushed the jochiari/cutom-props branch from c20b178 to ef48261 Compare April 4, 2018 01:40
@joeuy
Copy link
Copy Markdown
Contributor Author

joeuy commented Apr 4, 2018

@micahgodbolt anything else required to merge? Thanks!

@micahgodbolt micahgodbolt merged commit d77d755 into microsoft:master Apr 4, 2018
@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.

3 participants