dbeaver/pro#7251 feat: add skip navigation feature for improved acces…#4245
dbeaver/pro#7251 feat: add skip navigation feature for improved acces…#4245SychevAndrey wants to merge 6 commits intodevelfrom
Conversation
…r SkipNavLinks and SkipNavShortcutsLink
|
|
|
||
| import { PANEL_ID_LEFT_SIDEBAR, PANEL_ID_MAIN_CONTENT } from './AppScreenService.js'; | ||
| import { SkipNavService } from './SkipNavService.js'; | ||
| import './SkipNavLinks.css'; |
There was a problem hiding this comment.
please use css modules, dont import css like that
There was a problem hiding this comment.
skip-nav styles are inherently global: SkipNavShortcutsLink in plugin-help needs the same styles as the main container. With CSS modules this will leed to duplicating the same styles in two .module.css files (in plugin-focus-skip and plugin-help), whereas a single plain CSS file with a BEM class covered both components with zero duplication.
There was a problem hiding this comment.
We are not going to use styles like this. We want to stick with css modules. Please be consistent with a codebase patterns, if you want to reuse the same styles, you can useexport { default as CellStyles } from './Cell.module.css'; Or you can use tailwind
There was a problem hiding this comment.
i agree, that in this case it's better to use tailwind css or css-modules, because this components does not differ much from our regular components in the app
|
|
||
| return ( | ||
| <nav aria-label={translate('app_skip_nav_label')} className="dbv-kit-skip-nav"> | ||
| <button type="button" className="dbv-kit-skip-nav__link" onClick={() => focusPanel(PANEL_ID_LEFT_SIDEBAR)}> |
There was a problem hiding this comment.
please use UnstyledButton instead so it will have extended accesibility features
There was a problem hiding this comment.
What accessibility features UnstyledButton adds on top of what a native button already provides?
There was a problem hiding this comment.
You can keep native button but if we will have the desire to change something about all unstyled buttons in the app, we will not be able to
| } | ||
|
|
||
| return ( | ||
| <button type="button" className="dbv-kit-skip-nav__link" onClick={handleClick}> |
There was a problem hiding this comment.
even ariakit docs suggest using native button instead their component
There was a problem hiding this comment.
I think they are talking about render prop, you can render button as different html element, they suggest to keep it native. Ariakit uses native button underneath, so they are equal in terms of browser support
| const SkipNavLinks = importLazyComponent(() => import('./SkipNavLinks.js').then(m => m.SkipNavLinks)); | ||
|
|
||
| @injectable(() => [AppScreenService]) | ||
| export class SkipNavBootstrap extends Bootstrap { |
There was a problem hiding this comment.
Move it to a separate plugin, plugin-focus-skip, mention "focus" also in the file names
There was a problem hiding this comment.
skip-nav is essentially part of the app shell layout (just like TopNavBar, which also lives in appScreenService.placeholder). It's 3 files of actual code. Extracting it into a standalone package results in ~10 boilerplate files for 3 files of logic, plus a new dependency from plugin-help to plugin-focus-skip.
There was a problem hiding this comment.
As a general tip, if you are creating a bootstrap class, it is a good indication that the logic you are implementing should be in a separate plugin. The number of lines does not matter, we have plugins with even fewer lines. Our goal is to keep the app modular, so you should be able to remove a plugin from the declaration file and have the feature removed along with it.
There was a problem hiding this comment.
i think it's better to keep it here for now
There was a problem hiding this comment.
so TopNavBar should be a separate plugin as well?
There was a problem hiding this comment.
You can create a method inside NavService, something like registerLinks and call it from AppScreenBootstrap register method so we only have one bootstrap per package
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
TIP This summary will be updated as you push new changes. Give us feedback
| display: flex; | ||
| } | ||
|
|
||
| .dbv-kit-skip-nav__link { |
There was a problem hiding this comment.
Why we use dbv kit prefixes here?
| export function SkipNavShortcutsLink(): React.ReactElement { | ||
| const translate = useTranslate(); | ||
| const commonDialogService = useService(CommonDialogService); |
|
|
||
| export function SkipNavLinks(): React.ReactElement { | ||
| const translate = useTranslate(); | ||
| const skipNavService = useService(SkipNavService); |
There was a problem hiding this comment.
you should use observable with useTranslate
| const SkipNavLinks = importLazyComponent(() => import('./SkipNavLinks.js').then(m => m.SkipNavLinks)); | ||
|
|
||
| @injectable(() => [AppScreenService]) | ||
| export class SkipNavBootstrap extends Bootstrap { |
There was a problem hiding this comment.
i think it's better to keep it here for now
|
|
||
| import { PANEL_ID_LEFT_SIDEBAR, PANEL_ID_MAIN_CONTENT } from './AppScreenService.js'; | ||
| import { SkipNavService } from './SkipNavService.js'; | ||
| import './SkipNavLinks.css'; |
There was a problem hiding this comment.
i agree, that in this case it's better to use tailwind css or css-modules, because this components does not differ much from our regular components in the app
4 participants
…sibility
Screen.Recording.2026-03-27.at.17.58.23.mov