-
Notifications
You must be signed in to change notification settings - Fork 31
chore(GamutProvider): useLogicalProperties hook #3306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 13 commits
7f034fa
108f425
c0b93a2
e9dae8d
1dcc89a
f873f06
37924f5
4f89003
afe99d5
058aede
caf7514
3939c6f
e0b009f
4dd96c0
2b40dea
ef51010
87977e8
264423e
8975574
80f56ab
550e5e9
29ef404
1fe7c8c
c5ad7fe
373e16e
46a2950
f4c3f38
56011cb
efe7e58
3c6a8ee
6efbb0b
2bb07ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| gamut-styles: minor | ||
| --- | ||
|
|
||
| feat(gamut-styles): add useLogicalProperties hook |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { elementDir } from '../elementDir'; | ||
|
|
||
| describe('elementDir', () => { | ||
| afterEach(() => { | ||
| document.documentElement.removeAttribute('dir'); | ||
| }); | ||
|
|
||
| it('returns rtl when the element has dir="rtl"', () => { | ||
| const el = document.createElement('div'); | ||
| el.setAttribute('dir', 'rtl'); | ||
| expect(elementDir(el)).toBe('rtl'); | ||
| }); | ||
|
|
||
| it('returns ltr when the element has dir="ltr"', () => { | ||
| const el = document.createElement('div'); | ||
| el.setAttribute('dir', 'ltr'); | ||
| expect(elementDir(el)).toBe('ltr'); | ||
| }); | ||
|
|
||
| it('falls back to documentElement dir when computed style is empty (JSDOM)', () => { | ||
| const el = document.createElement('div'); | ||
| document.documentElement.setAttribute('dir', 'rtl'); | ||
| expect(elementDir(el)).toBe('rtl'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import * as React from 'react'; | ||
|
|
||
| import { setupRtl } from '../../__tests__/testUtils'; | ||
| import { useElementDir } from '../elementDir'; | ||
|
|
||
| const DirProbe: React.FC = () => ( | ||
| <span data-testid="dir">{useElementDir()}</span> | ||
| ); | ||
|
|
||
| const WithRef: React.FC = () => { | ||
| const ref = React.useRef<HTMLDivElement>(null); | ||
| return ( | ||
| <> | ||
| <div data-testid="dir">{useElementDir(ref)}</div> | ||
| <div dir="rtl" ref={ref}> | ||
| x | ||
| </div> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const WithSpanRef: React.FC = () => { | ||
| const ref = React.useRef<HTMLSpanElement>(null); | ||
| return ( | ||
| <> | ||
| <span data-testid="dir">{useElementDir(ref)}</span> | ||
| <span dir="rtl" ref={ref}> | ||
| x | ||
| </span> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const renderDirProbe = setupRtl(DirProbe, {}); | ||
| const renderWithRef = setupRtl(WithRef, {}); | ||
| const renderWithSpanRef = setupRtl(WithSpanRef, {}); | ||
|
|
||
| describe('useElementDir', () => { | ||
| afterEach(() => { | ||
| document.documentElement.removeAttribute('dir'); | ||
| }); | ||
|
|
||
| it('returns rtl when documentElement has dir=rtl and no ref', () => { | ||
| document.documentElement.setAttribute('dir', 'rtl'); | ||
|
|
||
| const { view } = renderDirProbe(); | ||
| expect(view.getByTestId('dir')).toHaveTextContent('rtl'); | ||
| }); | ||
|
|
||
| it('returns ltr when documentElement has dir=ltr and no ref', () => { | ||
| document.documentElement.setAttribute('dir', 'ltr'); | ||
|
|
||
| const { view } = renderDirProbe(); | ||
| expect(view.getByTestId('dir')).toHaveTextContent('ltr'); | ||
| }); | ||
|
|
||
| it('uses ref target when it is an Element', () => { | ||
| const { view } = renderWithRef(); | ||
| expect(view.getByTestId('dir')).toHaveTextContent('rtl'); | ||
| }); | ||
|
|
||
| it('accepts non-div element refs (e.g. span)', () => { | ||
| const { view } = renderWithSpanRef(); | ||
| expect(view.getByTestId('dir')).toHaveTextContent('rtl'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { Theme, ThemeProvider } from '@emotion/react'; | ||
| import * as React from 'react'; | ||
|
|
||
| import { setupRtl } from '../../__tests__/testUtils'; | ||
| import { coreTheme as theme } from '../../themes'; | ||
| import { useLogicalProperties } from '../useLogicalProperties'; | ||
|
|
||
| const ValueReadout: React.FC = () => <span>{`${useLogicalProperties()}`}</span>; | ||
|
|
||
| const HookProbe: React.FC<{ themeForHook?: Theme }> = ({ | ||
| themeForHook = theme, | ||
| }) => ( | ||
| <ThemeProvider theme={themeForHook}> | ||
| <ValueReadout /> | ||
| </ThemeProvider> | ||
| ); | ||
|
|
||
| const renderView = setupRtl(HookProbe, {}); | ||
|
|
||
| describe('useLogicalProperties', () => { | ||
| it('returns false when theme sets useLogicalProperties: false', () => { | ||
| const { view } = renderView({ | ||
| themeForHook: { ...theme, useLogicalProperties: false }, | ||
| }); | ||
|
|
||
| view.getByText('false'); | ||
| }); | ||
|
|
||
| it('returns true when theme sets useLogicalProperties: true', () => { | ||
| const { view } = renderView({ | ||
| themeForHook: { ...theme, useLogicalProperties: true }, | ||
| }); | ||
|
|
||
| view.getByText('true'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import type { RefObject } from 'react'; | ||
| import { useEffect, useLayoutEffect, useReducer } from 'react'; | ||
|
|
||
| /** Resolved HTML `dir` keyword: effective writing direction after `dir`, then CSS `direction`, then document root. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: could go over the comments on this file with the robot to refactor them in JSDoc styled comments |
||
| export type DirValue = 'rtl' | 'ltr'; | ||
|
|
||
| /** | ||
| * Resolves the effective `dir` for an element (`rtl` or `ltr`), including JSDOM where | ||
| * `getComputedStyle(el).direction` is often empty while `dir` is set on the root. | ||
| */ | ||
| export function elementDir(el: Element): DirValue { | ||
| const ownDir = el.getAttribute('dir'); | ||
| if (ownDir === 'rtl') return 'rtl'; | ||
| if (ownDir === 'ltr') return 'ltr'; | ||
| const { direction } = getComputedStyle(el); | ||
| if (direction === 'rtl' || direction === 'ltr') { | ||
| return direction; | ||
| } | ||
| return document.documentElement.getAttribute('dir') === 'rtl' ? 'rtl' : 'ltr'; | ||
| } | ||
|
|
||
| /** | ||
| * Ref whose `current` may be any DOM {@link Element} subclass (`HTMLElement`, `SVGElement`, | ||
| * `HTMLButtonElement`, etc.). For structural/minimal types (e.g. tests), intersect with | ||
| * `Element` so `current` is still typed as an `Element` (e.g. `Pick<HTMLAnchorElement, 'id'> & Element`). | ||
| */ | ||
| export type ElementDirRef<T extends Element = Element> = RefObject<T | null>; | ||
|
|
||
| function resolveElement<T extends Element>( | ||
| elementRef: ElementDirRef<T> | undefined | ||
| ): Element { | ||
| return elementRef?.current instanceof Element | ||
| ? elementRef.current | ||
| : document.documentElement; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the effective `dir` for the resolved element (`rtl` or `ltr`), and updates when `dir` | ||
| * changes on the document subtree or after layout (so `ref.current` is current). | ||
| * Resolution uses {@link elementDir}. | ||
| * | ||
| * @param elementRef - Optional ref; when missing or `current` is not an `Element`, uses `document.documentElement`. | ||
| */ | ||
| export function useElementDir<T extends Element = Element>( | ||
| elementRef?: ElementDirRef<T> | ||
| ): DirValue { | ||
| const [, bump] = useReducer((n: number) => n + 1, 0); | ||
|
|
||
| useLayoutEffect(() => { | ||
| bump(); | ||
| }, [elementRef]); | ||
|
|
||
| useEffect(() => { | ||
| const observer = new MutationObserver(() => { | ||
| bump(); | ||
| }); | ||
| observer.observe(document.documentElement, { | ||
| attributeFilter: ['dir'], | ||
| attributes: true, | ||
| subtree: true, | ||
| }); | ||
| return () => observer.disconnect(); | ||
| }, []); | ||
|
|
||
| if (typeof document === 'undefined') { | ||
| return 'ltr'; | ||
| } | ||
|
|
||
| return elementDir(resolveElement(elementRef)); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,3 @@ | ||
| export * from './elementDir'; | ||
| export * from './themed'; | ||
| export * from './useLogicalProperties'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { useTheme } from '@emotion/react'; | ||
|
|
||
| /** | ||
| * Whether Gamut system props emit logical CSS properties (`marginInlineStart`, etc.) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "emit" feels a little off to me, maybe "map to"? or "resolve to"? |
||
| * vs physical (`marginLeft`, etc.). | ||
| * | ||
| * `GamutProvider` always merges an explicit boolean (default `false`). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noting that this should change later when the default is also changed |
||
| */ | ||
| export function useLogicalProperties() { | ||
| const theme = useTheme(); | ||
| return theme?.useLogicalProperties; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| import { system } from '@codecademy/gamut-styles'; | ||
| import { | ||
| system, | ||
| useElementDir, | ||
| useLogicalProperties, | ||
| } from '@codecademy/gamut-styles'; | ||
| import { variance } from '@codecademy/variance'; | ||
| import styled from '@emotion/styled'; | ||
| import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; | ||
|
|
@@ -50,35 +54,14 @@ | |
| const parent = containers?.parent; | ||
|
|
||
| // Memoize scrolling parents to avoid expensive DOM traversals | ||
| const scrollingParents = useScrollingParents( | ||
| targetRef as React.RefObject<HTMLElement | null> | ||
| ); | ||
| const scrollingParents = useScrollingParents(targetRef); | ||
|
|
||
| // Keep onRequestClose ref up to date | ||
| useEffect(() => { | ||
| onRequestCloseRef.current = onRequestClose; | ||
| }, [onRequestClose]); | ||
|
|
||
| // Detect RTL direction from the target element and watch for attribute changes so the | ||
| // position recalculates when changes occur | ||
| const [isRtl, setIsRtl] = useState(false); | ||
| useEffect(() => { | ||
| const checkDirection = () => { | ||
| const target = targetRef?.current; | ||
| const el = target instanceof Element ? target : document.documentElement; | ||
| setIsRtl(getComputedStyle(el).direction === 'rtl'); | ||
| }; | ||
|
|
||
| checkDirection(); | ||
|
|
||
| const observer = new MutationObserver(checkDirection); | ||
| observer.observe(document.documentElement, { | ||
| attributes: true, | ||
| attributeFilter: ['dir'], | ||
| subtree: true, | ||
| }); | ||
| return () => observer.disconnect(); | ||
| }, [targetRef]); | ||
| const isRtl = useElementDir(targetRef) === 'rtl'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for doing this! And was doing some clean-up here: #3317 but I like your type clean-up more :) will close mine
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is to say, I'd say assign yourself and mark as done if no one gets back to you re: inverting |
||
|
|
||
| const popoverPosition = useMemo(() => { | ||
| if (parent !== undefined) { | ||
|
|
@@ -95,6 +78,10 @@ | |
| return { styles: {}, physicalStyles: undefined }; | ||
| }, [parent, x, y, offset, alignment, invertAxis, isRtl]); | ||
|
|
||
| // Log logical properties to the console TEST CODE | ||
| const logicalProperties = useLogicalProperties(); | ||
| console.log('dir', isRtl, 'logicalProperties', logicalProperties); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be cleaned up now |
||
|
|
||
| useEffect(() => { | ||
| const target = targetRef?.current; | ||
| if (!target) return; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was getting an error when trying to commit files with spaces, this fixes that.