fix(ListView): Fixed issue with hideCloseIcon not being respected - #298
Conversation
|
Screenshot of expanded content if you add hideCloseIcon=true to listviewitem. |
Pull Request Test Coverage Report for Build 1111
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1159
💛 - Coveralls |
| return ( | ||
| <div className={classes}> | ||
| {onClose && ( | ||
| {!hideCloseIcon && onClose && ( |
There was a problem hiding this comment.
The idea was that onClose would be undefined if hideCloseIcon was passed to the ListViewItem. The problem is that onClose is being defaulted 😞
We could check onClose !== noop rather than passing along the hideCloseIcon property
| heading={title} | ||
| description={description} | ||
| stacked={boolean('Stacked', false)} | ||
| hideCloseIcon={false} |
There was a problem hiding this comment.
Can we make this a knob?
Changed logic to use onclose instead of passing in hideCloseIcon to determine whether to show close icon
|
@jeff-phillips-18 thank you for the review and feedback. I implemented your suggestions and now have an updated screenshot. Thank you. |
| <ListViewGroupItemContainer | ||
| expanded={compoundExpanded} | ||
| onClose={hideCloseIcon ? undefined : onCloseCompoundExpand} | ||
| hideCloseIcon={hideCloseIcon} |
There was a problem hiding this comment.
No need to pass this now.
There was a problem hiding this comment.
@chalettu This one is still there but can be removed.
| <ListViewGroupItemContainer | ||
| expanded={expanded} | ||
| onClose={hideCloseIcon ? undefined : this.toggleExpanded} | ||
| hideCloseIcon={hideCloseIcon} |
There was a problem hiding this comment.
No need to pass this now.
|
@jeff-phillips-18 , I removed the parameter. Let me know if you have any more feedback. Thanks in advance |
|
@chalettu Still one left 😉 |
3400d27 to
cd076f5
Compare
|
@jeff-phillips-18 , got it. Removed the last one. Good catch. |
|
LGTM. thanks! |
4 participants
Fix for issue #277 where the hideCloseIcon property did not have an effect on expanded content.