carbon
carbon copied to clipboard
WIP: add floating-ui to popover POC
Closes #14493
Proof of concept, work in progress, adding floating-ui to popover for positioning.
Ongoing implementation/exploration notes
- [x] To be compliant with CSP inline-src styles need to be applied via CSSOM in a useEffect hook
- [x] Existing
alignoptions should be unified against the options available on floating-ui'splacement(naming based on logical properties) https://floating-ui.com/docs/computeposition#placement - [x] Optimize the CSSOM so it's not hardcoded style values
- [x] Place the floating-ui positioning behind a prop so users can turn it on (it should be off by default)
- [x] ~Remove existing placement styling~ (can we do this without a breaking change? if not, conditionally apply classes if the floating-ui positioning feature is turned on or not)
- Update: we can't do this without a breaking change. Styles need to remain as they are today for use in other framework implementations (web components, angular, svelte, etc). Instead, the implementation of
autoAlignwill focus on applying proper styles based onalignfor backwards compatibility whenautoAlignis off.
- Update: we can't do this without a breaking change. Styles need to remain as they are today for use in other framework implementations (web components, angular, svelte, etc). Instead, the implementation of
- [x] Implement the
flipvisibility optimizer (we won't want to useautoPlacement: https://floating-ui.com/docs/autoPlacement#conflict-with-flip) - [x] ~Implement
shiftvisibility optimizer~ Not needed based on current specs for popover - [ ] Implement offset positioning to account for the caret (and not when using isTabTip). We need to use https://floating-ui.com/docs/arrow for this so that offset/arrow calculations are taken into account for when the tooltip may overflow a boundary. This same reason is why styling for the caret must come from js and we can't use the existing caret placement styling (
transform: translate(...)) in scss.- [x] Caret/arrow placement will be done via the css classes when autoAlign is off
- [ ] Caret/arrow placement will need to be done via js using the
x/yvalues frommiddlewareData...const {x, y} = middlewareData.arrow; - [ ] Ensure the caret handles static sides and complies with the spec regarding caret alignment (it should be a few px from the bottom edge when in left-top, for instance). Essentially, the caret is not always center aligned in the existing implementation and the
autoAlignshould follow the same spec if possible. - [ ] Ensure
isTabTipworks as expected (eliminates the offset and removes the caret)
- [ ] Do we want to allow middlewares (offset, flip, shift, etc) to be configurable? Maybe via a new floatingUIProps prop?
- [ ] Complete any remaining TODO notes in the exploration (organize/rename the refs passed through the context, etc.)
- [ ] Do a pass for code organization and tidy things up
More broadly, once implementation firms up:
- [ ] Does
autoAlignfully comply with the design spec for popover? - [ ] Are there any regressions in components that rely on popover? Tooltip, ToggleTip, IconButton, Button w/ hasIconOnly, etc.
- [ ] Add tests to ensure reflow works as expected
- [ ] Is this worth unit testing since jsdom doesn't support ResiseObserver?
- [ ] How could we add VRT for this to ensure stability?
- [ ] Document
autoAlign, related props on storybook and the website - [ ] Figure out logical properties support
- Floating UI uses non-logical properties (left, top, etc). Some of these have logical property equivalents (insetInlineStart, insetBlockStart) that we could use. However, there are some styles applied by Floating UI that don't have an equivalent, notably
transformandtranslate. I'm unsure how this will behave in RTL. There are some proposals up but nothing has been added yet- https://github.com/w3c/fxtf-drafts/issues/311
- https://github.com/w3c/csswg-drafts/issues/1544
- Possible solutions:
autoAlignis experimental and we state it simply does not support RTL at the moment- The component detects when in RTL mode, and inverses any
transformandtranslatevalues in the writing mode direction ("inline" - left/right) in addition to using logical properties instead ofleft,top, etc. We would need to write an adapter to convert all non-logical properties to their logical counterpart:left->insetInlineStart, etc.- Here's an example of this using a custom property that changes depending on LTR/RTL, which sets a value of
scaleX(1)orscaleX(-1)to "inverse" the values oftransformX, etc. This might be cleaner than actually inversing the values of transformX:transformX(-100px)->transformX(100px)via multiplying by-1.
- Here's an example of this using a custom property that changes depending on LTR/RTL, which sets a value of
- Floating UI uses non-logical properties (left, top, etc). Some of these have logical property equivalents (insetInlineStart, insetBlockStart) that we could use. However, there are some styles applied by Floating UI that don't have an equivalent, notably
Deploy Preview for carbon-elements ready!
| Name | Link |
|---|---|
| Latest commit | 63aa6c1952c267e6ca7a487ecb7cc5a7ea5f42a5 |
| Latest deploy log | https://app.netlify.com/sites/carbon-elements/deploys/65aea87ae15a560008a2d2e9 |
| Deploy Preview | https://deploy-preview-14654--carbon-elements.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Deploy Preview for v11-carbon-react ready!
| Name | Link |
|---|---|
| Latest commit | 13ada6b0a9ba7f27319fb60f226e6979a3c46212 |
| Latest deploy log | https://app.netlify.com/sites/v11-carbon-react/deploys/65fb2096b28510000854a425 |
| Deploy Preview | https://deploy-preview-14654--v11-carbon-react.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
After approval but before merging I'll do a pass to remove some of the demo stories
This is looking awesome! I noticed a tiny issue with the Slug callout; seems to only be an issue with the side and bottom alignments. Seems to be off by 1px
Sorry for the delay here, I ended up having to update the align prop definition across the whole system for all components using Popover. Also fixed a bunch of TypeScript issues.
@tw15egan @alisonjoseph both of those issues you found should be fixed
@guidari could you double check the changes to ToggletipButton to ensure I didn't break anything you added in the interactive tag PR?
@tay1orjones All good with the changes in the ToggletipButton. There is no a11y errors 🚀
@laurenmrice Thank you for catching both of those! They should be fixed now.
Toggletip Experimental Auto Align story
- @tay1orjones The only thing I see now is that the label and the info icon is wrapping to two lines no matter how wide your screen size is. I am assuming that is a bug and the label should be on one line? The open toggletip itself looks fine, it is just the label I was wondering about.
@laurenmrice Thanks! I just pushed up a commit that fixes that issue.
I also removed the autoalign story from VRT for now because
- It's experimental
- It was larger than percy would snapshot, so it was just an empty snapshot file
I added the popover playground story to vrt instead.