Add titleView to SelectMenu #477
Conversation
|
Hey @notfelineit ! I think it might be better to provide a header render function. Tooltip in the header might be a fairly common use-case, but it also might be border-line too custom like you said. Thoughts? |
|
I think this may be too small of a change to trigger such major addition as header render function. What do you think about removing tooltip in favor of putting text below the header? Reason I'm suggesting this is to avoid 2 popovers at the same time (select menu and tooltip).
Either way, I'm good with this change 👍 |
| hasFilter, | ||
| close, | ||
| title, | ||
| tooltipContent, |
There was a problem hiding this comment.
Could you rename this prop to tooltip, so that it matches hasTooltip prop naming?
|
@vadimdemedes That's a good idea! Will it work with really long helper text? |
|
Really long text probably doesn't belong in tooltips either. That's usually an indicator that you should be using a different pattern. Most of the time tooltips are short phrases. |
|
@mshwery I agree 🤔 Before the tooltip, I had a subtitle (like Vadim's suggestion) with the following: |
| detailView, | ||
| emptyView, | ||
| headerHeight, |
There was a problem hiding this comment.
Should we provide a default value?
There was a problem hiding this comment.
Absorootintootinly
notfelineit
changed the title
| /> | ||
| </Pane> | ||
| )} | ||
| {hasTitle && titleView} |
There was a problem hiding this comment.
Hm, I think we should provide the below (removed) jsx as the default prop value for titleView. That way consumers of this component can continue using it as is.
The way you've moved the code around makes this a breaking change, technically.
src/select-menu/src/SelectMenu.js
Outdated
| @@ -128,6 +138,40 @@ export default class SelectMenu extends PureComponent { | |||
| return {} | |||
| } | |||
|
|
|||
| getTitleView = (close, title, headerHeight, titleView) => { | |||
There was a problem hiding this comment.
I think you could get rid of this code in favor pushing it into the component that actually uses titleView instead.
|
@mshwery was that what you meant? |
| @@ -36,26 +42,48 @@ export default class SelectMenuContent extends PureComponent { | |||
| static defaultProps = { | |||
| options: [], | |||
| hasTitle: true, | |||
| hasFilter: true | |||
| hasFilter: true, | |||
| titleView: ({ close, title, headerHeight }) => ( | |||
There was a problem hiding this comment.
Nit: I would pull this out into a separate component so the defaultProps are easier to scan.
But this is exactly what I was thinking of!
There was a problem hiding this comment.
Whoo! ok, I'll do that.
…ergreen into franny/fix-SelectMenu
…anny/fix-SelectMenu
This PR:
onFilterChangewarning in the console?icon option to the SelectMenu header. (Needed for some Protocols UI work)If this seems to specialized, and we should instead pass a render function for the header (or something else!) let me know :-)