-
Notifications
You must be signed in to change notification settings - Fork 64
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
LG-4516: deprecate justify="fit" from popover and related components #2474
LG-4516: deprecate justify="fit" from popover and related components #2474
Conversation
🦋 Changeset detectedLatest commit: 15e4bd0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 |
Size Change: -361 B (-0.03%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
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 seems fine to me. One small comment but mostly LG!
@@ -63,7 +63,7 @@ The popover component will be automatically positioned relative to its nearest p | |||
| ------------------ | ---------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------- | | |||
| `active` | `boolean` | Determines whether the Popover is active or inactive | `false` | | |||
| `align` | `'top'` \| `'bottom'` \| `'left'` \| `'right'` \| `'center-horizontal'` \| `'center-vertical'` | A string that determines the alignment of the popover relative to the `refEl`. | `'bottom'` | | |||
| `justify` | `'start'` \| `'middle'` \| `'end'` \| `'fit'` | A string that determines the justification of the popover relative to the `refEl`. Justification will be defined relative to the `align` prop | `'start'` | |
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.
Are we intentionally removing end
here as well?
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.
oops no that's a mistake. added that back
15a4e16
to
a3b0c52
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.
a3b0c52
to
15e4bd0
Compare
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
…2474) * Remove justify fit from @leafygreen-ui/popover * Remove justify fit usage in packages using Popover * Changeset
✍️ Proposed changes
Justify.Fit
from @leafygreen-ui/popover`Justify.Fit
usage from packages using<Popover>
Why remove this?
Justify.Fit
was initially introduced 4+ years ago in #295. It's currently used 3 times across MongoDB repos. This seems to be a sign that it isn't clear what it's intended to do within the defined API forjustify
.Both
align
andjustify
are intended to control positioning, butJustify.Fit
attempts to also control the sizing of the popover element in certain cases. Continuing to support this adds complexity when using Floating UI, so I think it's better to remove at this time. Existing users should be able to utilize other props combinations or custom styling to achieve an identical UI. This deprecation has been cleared with design.Floating UI has a size middleware that can be leveraged for this purpose if there are future requests for resizing functionality.
🎟 Jira ticket: LG-4516
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changes