carbon icon indicating copy to clipboard operation
carbon copied to clipboard

WIP: add floating-ui to popover POC

Open tay1orjones opened this issue 2 years ago • 2 comments

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 align options should be unified against the options available on floating-ui's placement (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 autoAlign will focus on applying proper styles based on align for backwards compatibility when autoAlign is off.
  • [x] Implement the flip visibility optimizer (we won't want to use autoPlacement: https://floating-ui.com/docs/autoPlacement#conflict-with-flip)
  • [x] ~Implement shift visibility 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/y values from middlewareData ... 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 autoAlign should follow the same spec if possible.
    • [ ] Ensure isTabTip works 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 autoAlign fully 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 transform and translate. 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:
      • autoAlign is experimental and we state it simply does not support RTL at the moment
      • The component detects when in RTL mode, and inverses any transform and translate values in the writing mode direction ("inline" - left/right) in addition to using logical properties instead of left, 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) or scaleX(-1) to "inverse" the values of transformX, etc. This might be cleaner than actually inversing the values of transformX: transformX(-100px) -> transformX(100px) via multiplying by -1.

tay1orjones avatar Sep 14 '23 18:09 tay1orjones

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Sep 14 '23 18:09 netlify[bot]

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jan 25 '24 19:01 netlify[bot]

After approval but before merging I'll do a pass to remove some of the demo stories

tay1orjones avatar Feb 26 '24 20:02 tay1orjones

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 2024-02-28 09 30 00

tw15egan avatar Feb 28 '24 14:02 tw15egan

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 avatar Mar 08 '24 17:03 tay1orjones

@tay1orjones All good with the changes in the ToggletipButton. There is no a11y errors 🚀

guidari avatar Mar 08 '24 18:03 guidari

@laurenmrice Thank you for catching both of those! They should be fixed now.

tay1orjones avatar Mar 19 '24 19:03 tay1orjones

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. Screenshot 2024-03-20 at 12 53 54 PM

laurenmrice avatar Mar 20 '24 16:03 laurenmrice

@laurenmrice Thanks! I just pushed up a commit that fixes that issue.

I also removed the autoalign story from VRT for now because

  1. It's experimental
  2. It was larger than percy would snapshot, so it was just an empty snapshot file

I added the popover playground story to vrt instead.

tay1orjones avatar Mar 20 '24 18:03 tay1orjones