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: remove clrFocusTrap in favor of @angular/cdk #431

Merged
merged 7 commits into from Dec 19, 2022
Merged

feat: remove clrFocusTrap in favor of @angular/cdk #431

merged 7 commits into from Dec 19, 2022

Conversation

kevinbuhmann
Copy link
Member

@kevinbuhmann kevinbuhmann commented Dec 8, 2022 •
edited

PR Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • [N/A] Docs have been added / updated (for bug fixes / features)
    • This feature is not documented.
  • [N/A] If applicable, have a visual design approval

PR Type

Remove undocumented feature.

What is the current behavior?

Various components use custom focus trap functionality, and this custom functionality is exported by the library.

What is the new behavior?

Various components use focus trap functionality from @angular/cdk, and the custom functionality is removed.

Does this PR introduce a breaking change?

Yes.

  • The @angular/cdk package is now a required peer dependency.
  • The clrFocusTrap directive has been removed. Please use the cdkTrapFocus directive from @angular/cdk instead.

@kevinbuhmann kevinbuhmann self-assigned this Dec 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022 •
edited

👋 @kevinbuhmann,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, checkout out a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal #clarity-support Slack channel

Thank you,

🤖 Clarity Release Bot

@bbogdanov
Copy link
Contributor

There is already a newer implementation of a focus trap in the code that proved itself useful.

https://github.com/vmware-clarity/ng-clarity/blob/main/projects/angular/src/utils/focus-trap/focus-trap.ts

What is the need of introducing a new package that provides the same thing and on top of that introducing a dependency we have no control of?

@kevinbuhmann
Copy link
Member Author

What is the need of introducing a new package that provides the same thing and on top of that introducing a dependency we have no control of?

This is about redundancy. I'm proposing eliminating custom code for things that Angular provides. I think it is unnecessary/redundant/inefficient for us to maintain custom implementations of drag/drop, focus trap, list key focus, etc. These things are provided by Angular. It's just not the best use of time/people/the team.

The Angular CDK is maintained by the same team that maintains the Angular framework packages, so in my view, it's just part of depending on Angular. As a user, I'd expect an Angular component library to use and rely on the Component Development Kit because that's what it's for.

Jinnie
Jinnie requested changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@Jinnie Jinnie left a comment

Choose a reason for hiding this comment

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

Looks great, apart from the demo page failing.
It may be demo-specific, as the modal is pre-open and inline, but it would be great if you can take a look.

projects/demo/src/app/modal/modal-trap.demo.html Show resolved Hide resolved
@bbogdanov
Copy link
Contributor

What is the need of introducing a new package that provides the same thing and on top of that introducing a dependency we have no control of?

This is about redundancy. I'm proposing eliminating custom code for things that Angular provides. I think it is unnecessary/redundant/inefficient for us to maintain custom implementations of drag/drop, focus trap, list key focus, etc. These things are provided by Angular. It's just not the best use of time/people/the team.

The Angular CDK is maintained by the same team that maintains the Angular framework packages, so in my view, it's just part of depending on Angular. As a user, I'd expect an Angular component library to use and rely on the Component Development Kit because that's what it's for.

I would advice testing these changes in a real application before the actual release cause this could be a real issue if someone depends on the span stuff that the clrFocusTrap uses to trap the user at the moment. I could assist with that.

@kevinbuhmann
Copy link
Member Author

I would advice testing these changes in a real application before the actual release cause this could be a real issue if someone depends on the span stuff that the clrFocusTrap uses to trap the user at the moment. I could assist with that.

If you could test cdkTrapFocus ahead of time, that would be great. I would expect it to work in any use case. It may not be a drop in replacement, but it should work.

There will be prereleases, but you don't need to wait for this to be released to adopt the CDK.

@kevinbuhmann
Copy link
Member Author

Looks great, apart from the demo page failing.
It may be demo-specific, as the modal is pre-open and inline, but it would be great if you can take a look.

@Jinnie: I just forgot to import the A11yModule for the static demo. It's fixed now.

@kevinbuhmann kevinbuhmann marked this pull request as draft December 12, 2022 13:55
@elesueur
Copy link
Contributor

elesueur commented Dec 12, 2022 •
edited

@kevinbuhmann Adding a dependency on @angular/cdk is a significant architectural decision for Clarity Angular.

In this initial step, your desire to avoid duplication of a smallish piece of code to deal with focus trapping could cause bundle sizes to increase in all consuming applications by over 100K (uncompressed) because the cdkFocusTrap directive is part of the @angular/cdk/a11y entrypoint. I would be interested to know how well the Angular 15 build system can tree-shake the unused code out. Have you done any experiments to see how the addition of this new dependency increases application bundle size?

@kevinbuhmann
Copy link
Member Author

@elesueur Angular is generally very well architected for tree shaking. It is certainly more tree-shakable that Clarity itself which is a problem I hope to tackle for v15 with should help offset this. Additionally, the focus trap is not the only piece of code I wish to replace with CDK. See my other PRs.

@kevinbuhmann
Copy link
Member Author

Adding a dependency on @angular/cdk is a significant architectural decision for Clarity Angular.

The CDK is built for exactly this use case: component library development. I don't see any reason why we should maintain custom code for things the framework provides for us.

@kevinbuhmann kevinbuhmann force-pushed the kevin/breaking/cdk-focus-trap branch 2 times, most recently from 6e227b7 to 95c4b8f Compare December 12, 2022 17:24
@kevinbuhmann
Copy link
Member Author

kevinbuhmann commented Dec 12, 2022 •
edited

My last two force pushes avoid importing all of A11yModule which results in about 6 kb in bundle savings. This makes the app bundle size smaller with the cdkTrapFocus than clrFocusTrap.

https://github.com/vmware-clarity/ng-clarity/compare/2fc35fd9155542c7dbe8abeb72af1f3173d88030..95c4b8f09f46670ded12f58965764a782000f0c6

@kevinbuhmann kevinbuhmann force-pushed the kevin/breaking/cdk-focus-trap branch 2 times, most recently from 5118fe6 to c28032b Compare December 12, 2022 17:46
@Jinnie
Copy link
Contributor

Jinnie commented Dec 13, 2022

I support what Kevin shared above.
For a team of our size and composition, it's really important to clean up and minimize our maintenance surface.
We expect even bigger benefits from the CDK if we're successful in replacing the many arrow key navigation solutions that we have all over Clarity, several of them custom, with their own specifics and problems, and at least two, which more generic and reusable.
We also have certain researches around column reordering and virtual scrolling, which are based on the CDK. Though they are far from productization yet, we may want to see them reprioritized at some point in future, as these are quite common requests we receive.

@kevinbuhmann kevinbuhmann marked this pull request as ready for review December 15, 2022 21:57
@kevinbuhmann
chore(header): group nav level tests
2a687ae
@kevinbuhmann
chore(modal): test for focus trap properly
f840a31 
The `debugElement.query` method returns `null` for not found.
The `null` value is considered "defined" in JavaScript, so `toBeDefined`
is not an appropriate matcher for this test.
@kevinbuhmann
chore(modal): rename focusable to focusTrap in test
99d83f1
@kevinbuhmann kevinbuhmann merged commit c3cabf0 into beta Dec 19, 2022
@kevinbuhmann kevinbuhmann deleted the kevin/breaking/cdk-focus-trap branch December 19, 2022 14:57
@github-actions
Copy link
Contributor

🎉 This PR is included in version 15.0.0-beta.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Jinnie Jinnie Jinnie approved these changes

Assignees

@kevinbuhmann kevinbuhmann

Labels
cla-not-required released on @beta type:breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kevinbuhmann @bbogdanov @elesueur @Jinnie @vmwclabot