-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: add default targetEl for callout #248
Conversation
…is done before recompute useEffect
|
packages/attention/src/component.tsx
Outdated
@@ -92,7 +93,7 @@ export function Attention(props: AttentionProps) { | |||
attentionEl.current = v; | |||
}, | |||
get targetEl() { | |||
return targetEl?.current; | |||
return targetElRef.current; |
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.
targetElRef can be null so maybe do targetElRef?.current
just so the null is handled
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.
Ah good catch! 👍
tests/components/TextAreaTest.tsx
Outdated
@@ -126,6 +126,7 @@ describe('TextArea component', () => { | |||
const textAreaRef = createRef<HTMLTextAreaElement>(); | |||
|
|||
// Render the TextArea component and pass the ref as forwardRef | |||
// eslint-disable-next-line no-unused-vars | |||
const { container } = render(<TextArea ref={textAreaRef} />); |
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.
This could just be (without the eslint-disable):
// Render the TextArea component and pass the ref as forwardRef
render(<TextArea ref={textAreaRef} />);
# [1.6.0-next.4](v1.6.0-next.3...v1.6.0-next.4) (2024-06-17) ### Bug Fixes * add default targetEl for callout ([#248](#248)) ([e297880](e297880))
# [1.6.0](v1.5.0...v1.6.0) (2024-07-03) ### Bug Fixes * add default targetEl for callout ([#248](#248)) ([e297880](e297880)) * bump core to fix slider ([#250](#250)) ([5192147](5192147)) * **slider:** Prevent onChange/onChangeAfter called on mount ([#253](#253)) ([84ddd64](84ddd64)) ### Features * add warp ds eslint config ([#238](#238)) ([d83f799](d83f799)) * slider onChangeAfter prop ([#247](#247)) ([fa1af5c](fa1af5c)) * **toggle:** add ReactNode type to toggle labels ([#244](#244)) ([abeff99](abeff99))
Fixes Jira WARP-527
targetEl
whencallout
istrue
andtargetEl
isundefined
.targetEl
:"useAutoUpdatePosition hook is using @warp-ds/core, which uses Floating-ui's computePosition(). Floating-ui's computePosition() requires a defined targetEl to be able to compute the attentionEl's position and the attentionEl's arrow position. When props.callout is true, we only need computePosition() to calculate the callout's arrow position. So, we create a default targetEl for callout that we can pass to the useAutoUpdatePosition hook, in order to avoid Floating-ui from throwing an error."
ml-8
to the callout stories.Tested: