Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(dialog, modal, popover, input-date-picker, input-time-picker, sheet): support stacked component sequential closing with escape #9231

Merged
merged 78 commits into from
Oct 4, 2024
Merged

feat(dialog, modal, popover, input-date-picker, input-time-picker, sheet): support stacked component sequential closing with escape #9231

merged 78 commits into from
Oct 4, 2024

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Apr 30, 2024

Related Issue: #6456

Summary

Support component sequential closing with escape by having focus-trap handle the stacking order.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Apr 30, 2024
driskull
driskull reviewed May 2, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Initial feedbackz

packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/sheet/sheet.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/demos/sheet.html Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/focusTrapComponent.ts Outdated Show resolved Hide resolved
@Elijbet Elijbet changed the title feat(sheet, modal, popover, tooltip, combobox, dropdown, date-picker): support stacked component sequential closing with escape feat(sheet, modal, popover): support stacked component sequential closing with escape May 2, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label May 16, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label May 16, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label May 24, 2024
@benelan benelan changed the base branch from main to dev June 10, 2024 09:13
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Jun 11, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jun 19, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Jun 20, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@Elijbet Elijbet removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 3, 2024
driskull
driskull approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

🎉 🌮 Nice work!

packages/calcite-components/src/components/popover/popover.tsx Outdated Show resolved Hide resolved
jcfranco
jcfranco reviewed Oct 4, 2024
View reviewed changes
packages/calcite-components/src/components/input-date-picker/input-date-picker.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/components/input-time-picker/input-time-picker.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/popover/popover.tsx
@@ -522,6 +529,22 @@ export class Popover
this.reposition(true);
};

private clickOutsideDeactivates = (event: MouseEvent): boolean => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm callbacks passed to connectFocusTrap options need to be this-bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed. Fails tests without.

packages/calcite-components/src/tests/stackedFocusTrap.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/globalStyles.e2e.ts Outdated
await page.waitForChanges();
expect(await inputPicker.getProperty("open")).toBe(true);

async function testEscapeAndCheckOpenState(elements: E2EElement[]) {
Copy link
Member

Choose a reason for hiding this comment

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

This is unresolved in the new test file.

packages/calcite-components/src/utils/focusTrapComponent.ts Show resolved Hide resolved
packages/calcite-components/src/tests/stackedFocusTrap.e2e.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/tests/stackedFocusTrap.e2e.ts Show resolved Hide resolved
jcfranco
jcfranco approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅
✅✔✅✅✅✔✅✔✅✅✅✔✅✅✔✔✔✅✅✔✔✔✅✅✔✔✔✅✔✅
✅✅✔✅✔✅✅✔✅✅✅✔✅✔✅✅✅✅✔✅✅✅✅✔✅✅✅✅✔✅
✅✅✅✔✅✅✅✔✅✅✅✔✅✅✔✔✅✅✅✔✔✅✅✅✔✔✅✅✔✅
✅✅✅✔✅✅✅✔✅✅✅✔✅✅✅✅✔✅✅✅✅✔✅✅✅✅✔✅✅✅
✅✅✅✔✅✅✅✅✔✔✔✅✅✔✔✔✅✅✔✔✔✅✅✔✔✔✅✅✔✅
✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅✅

excited

packages/calcite-components/src/tests/stackedFocusTrap.e2e.ts

const expectedElementId =
focusTrapOrderElements[i].id === "sheet"
? "sheet-button"
Copy link
Member

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 refactor PR, but it'd be helpful to add a comment explaining why we check for sheet-button when sheet is the next element in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it now.

@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 4, 2024
@Elijbet Elijbet merged commit eb22130 into dev Oct 4, 2024
13 checks passed
@Elijbet Elijbet deleted the elijbet/6456-component-closing-sequence branch October 4, 2024 23:08
@github-actions github-actions bot added this to the 2.13.1 patch milestone Oct 4, 2024
benelan added a commit that referenced this pull request Oct 7, 2024
@benelan
Merge remote-tracking branch 'origin/dev' into benelan/element-internals
8e29dea 
* origin/dev: (230 commits)
  chore: release next
  chore(sort-handle): add messages (#10474)
  feat(accordion-item): stretch slotted actions to fill its height (#9250)
  chore: release next
  feat(dialog, modal, popover, input-date-picker,  input-time-picker, sheet): support stacked component sequential closing with escape (#9231)
  chore: remove commented-out code (#10478)
  chore: add cssrem VSCode extension recommendation (#10300)
  docs(accordion-item): fix deprecation tag (#10479)
  chore: release next
  feat(stepper-item): update component's active state background color. (#10475)
  refactor: use `requestAnimationFrame` to replace `readTask` (#10432)
  chore: release next
  fix(tip): fix rendering tied to named-slot content (#10470)
  ci: compile estimate totals per milestone (#10442)
  chore: release next
  fix(modal): fix rendering tied to named-slot content (#10469)
  chore: release next
  fix(shell-center-row): fix rendering tied to named-slot content (#10451)
  fix(inline-editable): fix rendering tied to default slot content (#10456)
  fix(input, input-number, input-text): should not set slotted actions to be disabled (#10458)
  ...
@Elijbet
Copy link
Contributor Author

Elijbet commented Oct 7, 2024 •
edited
Loading

This is the gist of the test @DitwanP, @geospatialem.

Whenever a focus-trapping component opens, it traps focus and gets added to the stack. Pressing escape should deactivate each returning focus to the next component in the stack.

I recommend setting up a stack of all focus-trap components as open, mimicking the setup in the e2e, but have 2 separate stacks with popover containing either input-time-picker or input-date-picker, as you can't have both open at the same time.

Click outside also needs to be tested. It's usually set to false. For input-date-picker and input-time-picker closing with click outside is delegated to popover: https://github.com/Esri/calcite-design-system/pull/9231/files#diff-42c23e5a429472ae2ecb636c2ba383162681d40c5ff70c3f89be55d376052e32R1002 So whatever the expected behavior for popover is in this case.

@geospatialem geospatialem mentioned this pull request Oct 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

@driskull driskull driskull approved these changes

@benelan benelan Awaiting requested review from benelan

Assignees

@Elijbet Elijbet

Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2024-10-29 - Oct Milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Elijbet @jcfranco @driskull @geospatialem