feat(modals): introduce new TooltipModal component #828
Conversation
There was a problem hiding this comment.
There seems to be a bug I can reproduce with the example:
- Click to expand a step
- Scroll up until the TooltipModal shifts position to the top of the button
- Keep scrolling up and down
Result: dual scrollbars appear with inability to scroll the page until the user hits esc
|
|
||
| const COMPONENT_ID = 'modals.tooltip_modal.title'; | ||
|
|
||
| export const StyledTooltipModalTitle = styled.div.attrs({ |
There was a problem hiding this comment.
Looks like https://github.com/zendeskgarden/react-components/blob/main/packages/modals/src/styled/StyledHeader.ts could be factored to extend this styled component.
There was a problem hiding this comment.
I'm not sure these align as well as FooterItem does. TooltipModal isn't a variant of Modal. We would have to override several styles to make it fit. i.e. as Modal evolves we don't want TooltipModal to change with it
There was a problem hiding this comment.
For all of these comments, I was suggesting that the TooltipModal becomes the new baseline and Modal extends from that. I might've missed something in the comparison, but it seemed that all properties represented in TooltipModal were also represented (and nearly 100% the same) in Modal.
|
@Austin94 Looking good so far on the visual front. Do we have to retain the disabled button in Step 1? Feels a bit odd. Also, what would happen if the tooltip modal is implemented near the edge of a screen? Presumably it would adjust its position instead of cutting off? |
This was modeled after some in-product usages. I'll remove it for the first step.
Depending on the position value provided it would automatically change position or adjust itself against the body size to avoid a cutoff. |
|
Thanks @Austin94 ! One other question - I noticed the padding between the start of the arrows and the edge of the tooltip modal is smaller than what we have in the Sketch files - was this intentional? It looks kind of tight IMO compared to Sketch but I'd like to update the Sketch file to be consistent with code either way |
|
@m-lai I based the original sizing off of the large |
Cheers!! |
| position: static !important; /* [1] */ | ||
|
|
||
| padding: ${props => props.theme.space.base * 5}px; | ||
| width: 400px; |
There was a problem hiding this comment.
Hard coding a width here forces consumers to override CSS when 400px doesn't work for their context. Currently running into this situation using the tooltip modal in the color picker and color swatch dialogs.
There was a problem hiding this comment.
I believe the solution is to !important as we do with
Description
This PR introduces the new
TooltipModalcomponent. Overall, the structure and semantics align with the existingModalimplementation, with an updated focus on relative positioning.Detail
Testing
We have begun seeing
ts-jestwarnings due to our Node version. I've updated thetsconfig.test.jsonfile to resolve these warnings.Popper.js v2
This is the first package to consume Popper v2 and the updated
usePopperhook. Overall this feels like a significant improvement to Popper v1. There are some differences in how modifiers are applied, including default offsets. We now include a default8pxoffset in all directions (this is customizable by the consumer).The required testing syntax also differs from Popper v1, but seems more robust and requires no mocking.
Re-Positioning and maintaining focus
A key requirement for this component is to be re-positioned without a full DOM re-render. This allows focus to be retained within the focus-jail and stay located on any navigation buttons.
Checklist
designer as a reviewer)
yarn start)?bedrock)