-
Notifications
You must be signed in to change notification settings - Fork 77
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!: floating-ui elements no longer take up space when closed #10241
Conversation
#1381 should not be an issue since we are using #5694 seems to be an issue with this PR so I guess that would be a showstopper. The problem with not positioning off screen is that it will affect the size of the parent container. Maybe once we implement the dialog element and popover attribute it will not be an issue any longer. For now, I'll close this PR since it causes issues with drag and drop and elements positioned offscreen. |
Thanks for testing. Bummer about #5694. 😞 |
If I set the |
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 have a few follow-up comments, but this is great, @driskull! ✨💪✨
packages/calcite-components/src/components/dropdown/dropdown.e2e.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # packages/calcite-components/src/components/input-date-picker/input-date-picker.e2e.ts # packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx
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!
Could you update the PR title and description to match our breaking change formatting? For the breaking change footer, feel free to use notes from our 3.x. breaking change doc.
This can be merged after the Oct milestone wraps up.
import { CSS } from "./resources"; | ||
|
||
describe("calcite-popover", () => { | ||
describe("renders", () => { | ||
renders("calcite-popover", { visible: false, display: "block" }); | ||
renders("calcite-popover", { display: "block" }); | ||
renders(`<calcite-popover label="test" open reference-element="ref"></calcite-popover><div id="ref">😄</div>`, { |
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 can be done in a follow-up, but can you wrap one of these renders
in a describe
block? Nested tests need a separate block since our test helpers use the same names for describe
/it
calls.
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.
fixing
@@ -1848,6 +1848,13 @@ export const panelWithPopoverZIndex = (): string => | |||
</calcite-flow-item> | |||
</calcite-flow> | |||
</calcite-shell-panel> </calcite-shell | |||
><calcite-popover open reference-element="button" offset-distance="-50" offset-skidding="15" style="z-index: 100"> | |||
><calcite-popover | |||
overlay-positioning="fixed" |
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.
Do you know why this is needed now?
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.
Its not needed, ill revert the change. The panel inside the popover just needs a width here. Ill update the test.
# Conflicts: # packages/calcite-components/src/components/dropdown/dropdown.e2e.ts
) **Related Issue:** #7537 ## Summary - depends on #10241 - updates `action` to allow cursor to be overriden - lists should set the `label` property for the "Move to" sorting menu to have a name for the list. - adds `calcite-sort-handle` component to handle sorting and moving between lists for non mouse users - internal component - adds stories - adds tests - list-item - deprecates `dragSelected` property: no longer needed. - deprecates `calciteListItemDragHandleChange` event. no longer needed. - removed setting `aria-posinset` and `aria-setsize`. These are only needed when only part of the items are availalbe in the DOM. So we can safely remove these. - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-setsize - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-posinset - replaces `calcite-handle` with `calcite-sort-handle` in the `list` component. - updates tests - adds tests - adds demo pages BREAKING CHANGE: Modifies the component's keyboard sorting experience by using a dropdown menu to move list items. The ListItem `dragSelected` property and `calciteListItemDragHandleChange` event have been removed due to no longer being relevant.
@driskull I noticed this doesn't have a breaking change footer per our conventions. Can you use commit overrides in the description to include it (see https://github.com/Esri/calcite-design-system/wiki/monorepo#edit-changelog-entries)? |
@jcfranco added a breaking change to the PR body. Let me know if its good now. |
@driskull Looking good! Can you revisit the breaking change note with migration advice for developers? You can use what we have in our 3.0 breaking changes doc (migration plan note content):
Also, should we add a note to apply any dimension-altering styles the content instead of on the host? @benelan Could you review the override comment? Not sure if the override needs to include the whole description or just the title part. Once confirmed, we should update the monorepo wiki accordingly. |
Updated. Feel free to adjust the formatting or wording. |
Normally it's just the title, but in this case I'm not sure if the |
Related Issue: #10240
Summary
display:none
when not open.display:none
is removed in theonClose
of the component so that the animation can finish before its set to be hidden.Avoids closed floating ui elements from affecting layout changes. Example:
BREAKING CHANGE: Developers will need to make sure any floating-ui components within a scrolling container use
overlay-positioning="fixed"
and apply any dimension-altering styles to the slotted content instead of on the component host.BEGIN_COMMIT_OVERRIDE
fix!: Closed floating elements should not cause scrollbars
END_COMMIT_OVERRIDE