chore(inline-time-picker): support focus and focus traps (VIV-2296) #2146
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2146 +/- ##
============================================
Coverage 100.00% 100.00%
============================================
Files 123 353 +230
Lines 1562 16230 +14668
Branches 108 2916 +2808
============================================
+ Hits 1562 16230 +14668
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I commented on the changes for inline-time-picker.
We all agreed to work in small interations. That means, each PR should have a single purpose and as few files changed as possible.
To do that, we should finish this PR for the inline-time-picker focus handling, merge it, and then move on to using inline-time-picker. The idea is to merge what's ready.
| @@ -596,5 +596,33 @@ describe('vwc-inline-time-picker', () => { | |||
| expect(isScrolledIntoView(getPickerItem('hours', '00'))).toBe(true); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('focus trap support', () => { | |||
There was a problem hiding this comment.
IMO describes should describe API's.
I know there's more like this in our codebase. It should be refactored.
Anyway, it describes nothing but a single use case so should be removed and its it remain under keyboard interactions.
There was a problem hiding this comment.
Well, I think we should use describe to group related behaviour. This is similar to how we have describe("form association") in the codebase.
I don't want to move this under "time picker > keyboard navigation" as it has nothing to do with keyboard navigation for picking times
There was a problem hiding this comment.
What scenario does it test? What user action?
| TrappedFocus.ignoreEvent = originalIgnoreEvent; | ||
| }); | ||
|
|
||
| it('should submit Tab keydown events that move focus internally to be ignored by focus traps', () => { |
There was a problem hiding this comment.
What I see in this test is:
Focus on hours, press tab, expect ignoreEvent method to be called once with the Tab event.
Can you please write the description in the form of: should {doSomething} when {someThing}?
There was a problem hiding this comment.
And how will you understand the purpose without this description? I think it will not become clearer by phrasing it like that
libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.template.ts
Outdated
Show resolved
Hide resolved
libs/components/src/lib/time-picker/inline-time-picker/inline-time-picker.ts
Show resolved
Hide resolved
RichardHelm
force-pushed
the
VIV-2296-use-inline-time-picker
branch
from
6f2a7f9 to
0f60790
Compare
|
@YonatanKra Sure, I've split the PR |
RichardHelm
changed the title
No description provided.