Skip to content

Remove dom-helpers dependency from @react-aria/overlays#3291

Merged
devongovett merged 3 commits into
mainfrom
remove-dom-helpers
Jul 12, 2022
Merged

Remove dom-helpers dependency from @react-aria/overlays#3291
devongovett merged 3 commits into
mainfrom
remove-dom-helpers

Conversation

@devongovett
Copy link
Copy Markdown
Member

Reduces bundle size by ~1.8 KB.

@adobe-bot
Copy link
Copy Markdown

reidbarber
reidbarber previously approved these changes Jul 11, 2022
Comment on lines +405 to +411
function getOffset(node: Element): Offset {
let box: Offset = node.getBoundingClientRect();
let {scrollTop, scrollLeft, clientTop, clientLeft} = document.documentElement;
box.top += scrollTop - clientTop;
box.left += scrollLeft - clientLeft;
return box;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This errors when I attempt to open a Picker in the following story: https://reactspectrum.blob.core.windows.net/reactspectrum/7c18f4284ae7f6fd19e8b8be8da8ee28e82cc2b4/storybook/index.html?path=/story/picker--default&providerSwitcher-toastPosition=bottom. Similar thing happens which other overlay components (MenuTrigger, ComboBox, etc)

Comment thread yarn.lock
"@babel/runtime" "^7.1.2"

dom-helpers@^5.0.1, dom-helpers@^5.2.1:
dom-helpers@^5.0.1:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's odd there's still one left over, looks like it's a dependency of react-transition-group, which we have two different version dependencies on actually, even with the removal you did
Screen Shot 2022-07-11 at 9 56 41 AM
I'm happy to look into consolidating those separately, so this is mostly just a note

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, react-spectrum will still have a dependency, but react-aria won't

@adobe-bot
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, no crashes :)

Copy link
Copy Markdown
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, more complicated examples such as offset/cross offset/different containers/different targets all seem to be working as expected

@adobe-bot
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants