Copilot commented on code in PR #4308: URL: https://github.com/apache/streampipes/pull/4308#discussion_r2997836615
########## ui/src/app/help/components/shortcuts/shortcuts.component.html: ########## @@ -0,0 +1,60 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<div class="fixed-height" fxLayout="column" fxFlex="100"> + <div fxLayout="column" fxLayoutGap="10px"> + <sp-basic-header-title-component + title="Shortcuts" + ></sp-basic-header-title-component> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + S' in chart/dashboard/pipeline edit view" + fxFlex="100" + > + Saves the current state + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'E' in dashboard/pipeline panel" + fxFlex="100" + > + Enters edit mode. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Delete/Backspace' in pipeline editor" + fxFlex="100" + > + Deletes the currently hovered pipeline element. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + F' in table widget filter dropdown (charts type -> table)" + fxFlex="100" + > + Focuses/selects the filter search input. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Esc' Dialog keyboard behavior" Review Comment: The `panelTitle` value includes extra quotes (e.g., starts with `'Ctrl/Cmd + S' ...`), which will likely be rendered literally in the UI. If you just want the shortcut text emphasized/quoted, pass a plain string (without the outer quotes) or format it in the panel content instead. ```suggestion panelTitle="Ctrl/Cmd + S in chart/dashboard/pipeline edit view" fxFlex="100" > Saves the current state </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="E in dashboard/pipeline panel" fxFlex="100" > Enters edit mode. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Delete/Backspace in pipeline editor" fxFlex="100" > Deletes the currently hovered pipeline element. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Ctrl/Cmd + F in table widget filter dropdown (charts type -> table)" fxFlex="100" > Focuses/selects the filter search input. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Esc Dialog keyboard behavior" ``` ########## ui/src/app/help/components/shortcuts/shortcuts.component.html: ########## @@ -0,0 +1,60 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<div class="fixed-height" fxLayout="column" fxFlex="100"> + <div fxLayout="column" fxLayoutGap="10px"> + <sp-basic-header-title-component + title="Shortcuts" + ></sp-basic-header-title-component> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + S' in chart/dashboard/pipeline edit view" + fxFlex="100" + > + Saves the current state + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'E' in dashboard/pipeline panel" + fxFlex="100" + > + Enters edit mode. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Delete/Backspace' in pipeline editor" + fxFlex="100" + > + Deletes the currently hovered pipeline element. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + F' in table widget filter dropdown (charts type -> table)" + fxFlex="100" + > + Focuses/selects the filter search input. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Esc' Dialog keyboard behavior" Review Comment: Same issue as above: the `panelTitle` includes outer quotes that will likely render literally (e.g., `"'E' ..."`). Prefer a plain string title and handle any quoting/styling in the displayed content. ```suggestion panelTitle="Ctrl/Cmd + S in chart/dashboard/pipeline edit view" fxFlex="100" > Saves the current state </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="E in dashboard/pipeline panel" fxFlex="100" > Enters edit mode. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Delete/Backspace in pipeline editor" fxFlex="100" > Deletes the currently hovered pipeline element. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Ctrl/Cmd + F in table widget filter dropdown (charts type -> table)" fxFlex="100" > Focuses/selects the filter search input. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Esc Dialog keyboard behavior" ``` ########## ui/src/app/core/components/toolbar/toolbar.component.html: ########## @@ -123,24 +123,12 @@ </button> <button mat-menu-item - (click)="openInfo()" + (click)="openHelp()" style="min-width: 0px" > <mat-icon aria-label="Info">help</mat-icon> Review Comment: The Help menu item’s icon has `aria-label="Info"`, which is misleading for screen readers. Update it to something accurate like "Help" (or move the accessible label to the button and keep the icon decorative). ```suggestion <mat-icon aria-hidden="true">help</mat-icon> ``` ########## ui/src/app/help/help.component.html: ########## @@ -0,0 +1,61 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<sp-basic-view> + <div fxLayout="row" nav> + <div fxFlex="100"> + <div fxFlex="100" fxLayout="row"> + <div fxFlex fxLayoutAlign="start center" [attr.id]="'helpTabs'"> + <mat-tab-group + [selectedIndex]="selectedIndex" + (selectedIndexChange)="selectedIndexChange($event)" + color="accent" + > + <mat-tab label="Info"></mat-tab> + <mat-tab label="Documentation"></mat-tab> + <mat-tab label="Shortcuts"></mat-tab> + </mat-tab-group> Review Comment: The Documentation tab is always shown, even when `documentationLink` is empty/unconfigured. That makes the "View Documentation" action lead to an empty/relative link. Consider conditionally showing the Documentation tab/button only when a valid URL is configured (and/or when the corresponding link setting enables it), or display a clear "Documentation link not configured" state. ########## ui/src/app/help/components/shortcuts/shortcuts.component.html: ########## @@ -0,0 +1,60 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<div class="fixed-height" fxLayout="column" fxFlex="100"> + <div fxLayout="column" fxLayoutGap="10px"> + <sp-basic-header-title-component + title="Shortcuts" + ></sp-basic-header-title-component> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + S' in chart/dashboard/pipeline edit view" + fxFlex="100" + > + Saves the current state + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'E' in dashboard/pipeline panel" + fxFlex="100" + > + Enters edit mode. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Delete/Backspace' in pipeline editor" + fxFlex="100" + > + Deletes the currently hovered pipeline element. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + F' in table widget filter dropdown (charts type -> table)" + fxFlex="100" + > + Focuses/selects the filter search input. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Esc' Dialog keyboard behavior" Review Comment: Same issue as above: the `panelTitle` contains outer quotes that will likely be shown in the UI (`"'Delete/Backspace' ..."`). ```suggestion panelTitle="Ctrl/Cmd + S in chart/dashboard/pipeline edit view" fxFlex="100" > Saves the current state </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="E in dashboard/pipeline panel" fxFlex="100" > Enters edit mode. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Delete/Backspace in pipeline editor" fxFlex="100" > Deletes the currently hovered pipeline element. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Ctrl/Cmd + F in table widget filter dropdown (charts type -> table)" fxFlex="100" > Focuses/selects the filter search input. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Esc Dialog keyboard behavior" ``` ########## ui/src/app/help/components/shortcuts/shortcuts.component.html: ########## @@ -0,0 +1,60 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<div class="fixed-height" fxLayout="column" fxFlex="100"> + <div fxLayout="column" fxLayoutGap="10px"> + <sp-basic-header-title-component + title="Shortcuts" + ></sp-basic-header-title-component> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + S' in chart/dashboard/pipeline edit view" + fxFlex="100" + > + Saves the current state + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'E' in dashboard/pipeline panel" + fxFlex="100" + > + Enters edit mode. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Delete/Backspace' in pipeline editor" + fxFlex="100" + > + Deletes the currently hovered pipeline element. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + F' in table widget filter dropdown (charts type -> table)" + fxFlex="100" + > + Focuses/selects the filter search input. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Esc' Dialog keyboard behavior" Review Comment: Same issue as above: `panelTitle` contains outer quotes and will likely render them literally (`"'Ctrl/Cmd + F' ..."`). ```suggestion panelTitle="Ctrl/Cmd + S in chart/dashboard/pipeline edit view" fxFlex="100" > Saves the current state </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="E in dashboard/pipeline panel" fxFlex="100" > Enters edit mode. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Delete/Backspace in pipeline editor" fxFlex="100" > Deletes the currently hovered pipeline element. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Ctrl/Cmd + F in table widget filter dropdown (charts type -> table)" fxFlex="100" > Focuses/selects the filter search input. </sp-basic-inner-panel> <sp-basic-inner-panel panelTitle="Esc Dialog keyboard behavior" ``` ########## ui/src/app/help/components/shortcuts/shortcuts.component.html: ########## @@ -0,0 +1,60 @@ +<!-- +~ Licensed to the Apache Software Foundation (ASF) under one or more +~ contributor license agreements. See the NOTICE file distributed with +~ this work for additional information regarding copyright ownership. +~ The ASF licenses this file to You under the Apache License, Version 2.0 +~ (the "License"); you may not use this file except in compliance with +~ the License. You may obtain a copy of the License at +~ +~ http://www.apache.org/licenses/LICENSE-2.0 +~ +~ Unless required by applicable law or agreed to in writing, software +~ distributed under the License is distributed on an "AS IS" BASIS, +~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +~ See the License for the specific language governing permissions and +~ limitations under the License. +~ +--> + +<div class="fixed-height" fxLayout="column" fxFlex="100"> + <div fxLayout="column" fxLayoutGap="10px"> + <sp-basic-header-title-component + title="Shortcuts" + ></sp-basic-header-title-component> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + S' in chart/dashboard/pipeline edit view" + fxFlex="100" + > + Saves the current state + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'E' in dashboard/pipeline panel" + fxFlex="100" + > + Enters edit mode. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Delete/Backspace' in pipeline editor" + fxFlex="100" + > + Deletes the currently hovered pipeline element. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Ctrl/Cmd + F' in table widget filter dropdown (charts type -> table)" + fxFlex="100" + > + Focuses/selects the filter search input. + </sp-basic-inner-panel> + + <sp-basic-inner-panel + panelTitle="'Esc' Dialog keyboard behavior" Review Comment: Same issue as above: `panelTitle` contains outer quotes (`"'Esc' ..."`) which will likely be rendered literally. ```suggestion panelTitle="Esc dialog keyboard behavior" ``` ########## ui/src/app/help/help.component.ts: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +import { Component, OnInit, inject } from '@angular/core'; +import { + SpBasicViewComponent, + SpBreadcrumbService, +} from '@streampipes/shared-ui'; +import { + FlexDirective, + LayoutAlignDirective, + LayoutDirective, +} from '@ngbracket/ngx-layout/flex'; +import { MatTab, MatTabGroup } from '@angular/material/tabs'; +import { LoginService } from '../login/services/login.service'; +import { InfoTabComponent } from './components/info/info.component'; +import { DocumentationTabComponent } from './components/documentation/documentation.component'; +import { ShortcutsTabComponent } from './components/shortcuts/shortcuts.component'; + +@Component({ + selector: 'sp-help', + templateUrl: './help.component.html', + styleUrls: ['./help.component.scss'], + imports: [ + SpBasicViewComponent, + FlexDirective, + LayoutDirective, + LayoutAlignDirective, + MatTabGroup, + MatTab, + InfoTabComponent, + DocumentationTabComponent, + ShortcutsTabComponent, + ], +}) +export class HelpComponent implements OnInit { + private breadcrumbService = inject(SpBreadcrumbService); + private loginService = inject(LoginService); + + selectedIndex = 0; + documentationLink = ''; + + ngOnInit(): void { + this.breadcrumbService.updateBreadcrumb([{ label: 'Help' }]); + this.loginService.fetchLoginSettings().subscribe(res => { + this.documentationLink = res.linkSettings?.documentationUrl || ''; Review Comment: The previous UI respected `linkSettings.showDocumentationLinkInProfileMenu` (configurable under General Configuration → Links). After moving documentation into Help, that flag appears to be unused, and the Documentation tab will still be reachable even when admins disabled it. Consider also reading `showDocumentationLinkInProfileMenu` here and using it to hide/disable the Documentation tab (or remove/deprecate the setting if it’s intentionally no longer supported). ```suggestion const linkSettings = res.linkSettings; const showDocumentationLinkInProfileMenu = linkSettings?.showDocumentationLinkInProfileMenu; if (showDocumentationLinkInProfileMenu === false) { // Respect admin setting: hide/disable documentation by clearing the link this.documentationLink = ''; } else { this.documentationLink = linkSettings?.documentationUrl || ''; } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
