ref(alert): Only add onClick listeners to top message portion#33356
Conversation
| <Message>{children}</Message> | ||
| {(showExpand || showTrailingItems) && ( | ||
| <TrailingItemsWrap> | ||
| <TrailingItems onClick={e => e.stopPropagation()}> |
There was a problem hiding this comment.
stop propagating onClick events that come from elements inside the trailingItems area, since that's where we put buttons and links.
There was a problem hiding this comment.
Should the <ExpandIcon ... /> live inside the trailing items?
There was a problem hiding this comment.
yes, I want it to be aligned with the other trailing items, like so:
| const ExpandIcon = styled(IconChevron, { | ||
| shouldForwardProp: (p: string) => p !== 'isExpanded', | ||
| })<{isExpanded: boolean}>` | ||
| transform: ${p => (p.isExpanded ? 'rotate(0deg)' : 'rotate(180deg)')}; |
There was a problem hiding this comment.
👎 use the direction prop on the icon itself
There was a problem hiding this comment.
ahh that's from back when we had more styles than just the transform - totally didn't notice that we could just remove it now. did so in ca2ea1c!
| padding-top: ${space(1.5)}; | ||
| padding-bottom: ${space(1.5)}; | ||
| padding-left: ${space(2)}; | ||
| padding-right: ${space(1)}; |
There was a problem hiding this comment.
| padding-top: ${space(1.5)}; | |
| padding-bottom: ${space(1.5)}; | |
| padding-left: ${space(2)}; | |
| padding-right: ${space(1)}; | |
| padding: ${space(1.5)} ${space(1)} ${space(1.5)} ${space(2)}; |
🤷 nit
There was a problem hiding this comment.
most people know the top right bottom left rule
| ))` | ||
| transform: ${props => (props.isExpanded ? 'rotate(0deg)' : 'rotate(180deg)')}; | ||
| justify-self: flex-end; | ||
| const TrailingItemsWrap = styled(TrailingItems)` |
There was a problem hiding this comment.
any way to get rid of the wrap 🤔
There was a problem hiding this comment.
created two wraps because I wanted to stop event propagation inside <TrailingItems /> (the area containing the two buttons below), but not on IconChevron, which is in <TrailingItemsWrap /> but not in <TrailingItems />.
so one way to make this simpler is to apply the grid styles to <TrailingItemsWrap />, and then just add display: contents to <TrailingItems />. This way, everything inside <TrailingItems /> will be grid items. Problem with this is that there's only partial support for display: contents in Safari. This is what it looks like in Safari:
2 participants
Currently, if an alert is expandable, we add an
onClickevent listener on the entire alert. If the expanded alert content contains clickable elements, the click event will propagate to the alert element and close the alert. This is unexpected UX and needs to be fixed.This PR refactors the alert component and attaches onClick listeners to the top (unexpanded) portion of the alert. Clicking inside the expanded content no longer closes the alert.
Storybook demo.