PR: menu → focus-group refactoring #16
Conversation
focus-group.js
Outdated
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Newline at the end of the file is missed.
I can recommend installing EditorConfig extension to your text editor.
focus-group.js
Outdated
| } | ||
| } | ||
|
|
||
| export function findGroupNodeByEventTarget(eventTarget) { |
There was a problem hiding this comment.
Do we need to export them?
Removing export will reduce JS bundle size
There was a problem hiding this comment.
I use export for testing purposes only, probably there is a better way to test the method...
There was a problem hiding this comment.
Can we do integration testing instead (test the whole feature)?
There was a problem hiding this comment.
Sure I just don't know which strategy is better here... I will try to fix it today evening!
focus-group.js
Outdated
| } | ||
|
|
||
| export function findGroupNodeByEventTarget(eventTarget) { | ||
| let supportedRoles = { |
There was a problem hiding this comment.
We need to move it out of the function to const
focus-group.js
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you move those helpers above the place where we are using them?
focus-group.js
Outdated
| if (ariaOrientation === "vertical") return false | ||
| if (ariaOrientation === "horizontal") return true | ||
|
|
||
| if (!ariaOrientation) { |
There was a problem hiding this comment.
according to the doc the aria-orientation has 3 possible values:
Those if statement needs to handle the undefined or default value. If I understand it correctly in case of menubar or tablist it should be horizontal unless explicitly set
focus-group.js
Outdated
| if (!groupRoles) return null | ||
|
|
||
| let result | ||
| groupRoles.forEach((role) => { |
There was a problem hiding this comment.
It is better to use for of here and return on found. Because otherwise you will do unnecessary search for menubar if menu was already found.
index.d.ts
Outdated
| } | ||
|
|
||
| export interface MenuKeyUXOptions { | ||
| export interface focusGroupKeyUXOptions { |
Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>
….com/dmitry-kurmanov/keyux into feature/9-extend-menu-to-focus-group
focus-group.js
Outdated
| if (ariaOrientation === "vertical") return false | ||
| if (ariaOrientation === "horizontal") return true | ||
|
|
||
| if (!ariaOrientation) { |
There was a problem hiding this comment.
What case will we miss if we will write:
- if (!ariaOrientation) {
- let role = group.role
- return role === "menubar" || role === "tablist"
- }
- return false
+ let role = group.role
+ return role === "menubar" || role === "tablist"
focus-group.js
Outdated
| @@ -1,6 +1,12 @@ | |||
| export function menuKeyUX(options) { | |||
| const Roles = { | |||
There was a problem hiding this comment.
In my code style I use ROLES. Can you replace, please?
There was a problem hiding this comment.
Sure! My bad I missed it
|
Fix ESLint rules and that small notes and I think we could move PR from draft to ready state. |
|
Also, note the a little more proper way to make tab’s body Lines 308 to 313 in 294ef40 |
2 participants
without
toolboxsee #9