-
Notifications
You must be signed in to change notification settings - Fork 114
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
chore(utils): remove useMergeRef and add tests for useWindowSize #2719
Conversation
🦋 Changeset detectedLatest commit: 1aa09d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
59350b9
to
61771ab
Compare
8f77830
to
9dca18a
Compare
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.
LGTM!
70c7d9b
to
ccf386e
Compare
✅ Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1aa09d5:
|
Size Change: -168 B (0%) Total Size: 749 kB
ℹ️ View Unchanged
|
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Nice!
@@ -0,0 +1,6 @@ | |||
--- | |||
'@twilio-paste/utils': major | |||
'@twilio-paste/core': minor |
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'm imagining that utils are exposed through the same API from @twilio-paste/core
correct? So could do something like this:
import { useMergeRef } from '@twilio-paste/core
If that is true I'm not sure I follow how removing the hook would be a minor change? Perhaps I'm missing some nuance but it all feels like a breaking change and major bump?
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.
It is indeed exported through core, good callout! Ordinarily this would be a major to core, however I asked Nora to mark it as a minor for two reasons:
- The first and most important is that this is undocumented & internal tooling. Anything that isn't documented is Use At Your Own Risk. It's kind of like pre-v1.0.0 in semver, which means breaking changes don't need to bump the major version.
- Second no one is using this according to our reporting scans. So no need to worry folks for nothing.
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
73f1bb7
ccf386e
to
73f1bb7
Compare
73f1bb7
to
1aa09d5
Compare
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.
💪
DSYS-3564