fix(navbar-vertical): focus and active state handling#1512
fix(navbar-vertical): focus and active state handling#1512dauriamarco wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to focus and active state handling within the vertical navigation bar. The changes to si-navbar-vertical-group-trigger.directive.ts and si-navbar-vertical-group.component.ts correctly manage focus when flyouts are used and update to more modern Angular patterns. The critical issue identified in si-navbar-vertical-item.component.ts regarding the incorrect active state logic for 'action' type items has been retained as it does not contradict any existing rules and is a valid concern. A specific comment with a suggested fix for this issue is included.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts
Show resolved
Hide resolved
4beae6f to
062047d
Compare
062047d to
1b42630
Compare
|
Documentation. Coverage Reports: |
|
I think the focus handling is a general problem of the tooltip. Please have a look at #1517 |
@spike-rabbit Thank you, I like the solution proposed in #1517 👍 but I see it more as an alternative to #1514, which I opened to address more or less the same thing. Independently of the tooltip itself, the navbar issues still remain, including the incorrect focus behavior. The tooltip was mainly making the problem more visible, because it highlighted that focus was being restored to the wrong element. But even without showing the tooltip, the focus handling in the navbar as well as the active state are still not correct, at least from what I can see. |
spike-rabbit
left a comment
There was a problem hiding this comment.
The changes around active I understand. But I think the focus handling we can keep as is.
Those changes only have impact when using a mouse, but in those cases we don't really care where the focus is.
Closes #1511.