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

ComboBox: onRenderContainer and onRenderList need the default onRenderItem function #3050

Closed
abettadapur opened this issue Oct 6, 2017 · 5 comments · Fixed by #3059
Closed

Comments

@abettadapur
Copy link
Collaborator

@cschleiden

Describe the issue:

The onRenderContainer and onRenderList properties on the ComboBox are not particularly helpful because they are not passed the default render function for onRenderItem.

As an example, if I wanted to render the ComboBox list in the virtualized Fabric list, I have the following problems:

  • I do not have any of the state from ComboBox, so I cannot tell if an item is selected
  • I must rewrite the onRenderItem function for the List so that I can click on items in the list

It would be nice if the onRenderList function was passed the default render for the list as well as the default render functions for all of the components in the tree (onRenderItem, onRenderOption). That would solve the above two issues and make the onRenderList prop more useful.

@cschleiden
Copy link
Contributor

@christiango What do you think? We were thinking instead of just passing the ComboBox's props to onRenderList to pass at least this._onRenderItem.

@cschleiden cschleiden changed the title [ComboBox] onRenderContainer and onRenderList need the default onRenderItem function ComboBox: onRenderContainer and onRenderList need the default onRenderItem function Oct 6, 2017
@christiango
Copy link
Member

@jspurlin would be the best person to answer that, but it sounds reasonable to me.

@jspurlin
Copy link
Contributor

jspurlin commented Oct 6, 2017

Yeah, sounds reasonable, but if this change is made that callback will no longer be an IRenderFunction. Also, if we make this change we should return the actual onRenderItem (the default _onRenderItem or the custom render that was passed in, e.g. let { onRenderItem = this._onRenderItem } = this.props; and pass onRenderItem) otherwise you could get into a weird state where you try to use the onRenderItem that's passed to onRenderList and you don't get the desired result. If that didn't happen onRenderList would always have to check which onRenderItem they need to use

@cschleiden
Copy link
Contributor

Yes, of course. We should follow the "onRender resolution logic". For the onRenderList prop continuing to be an IRenderFunction, we could extend from the IComboBoxProps (e.g., IComboBoxRenderProps) and pass the resolved onRenderOption method in a new prop.

@abettadapur
Copy link
Collaborator Author

onRenderItem also needs to be passed some state about the item, so that I can mark it as selected in a custom implementation

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants