Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell: Even more types #21425

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

shell: Even more types #21425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97d27a5
shell: Improve manifest types
mvollmer Dec 11, 2024
de229c6
shell: Fully type "nav"
mvollmer Dec 11, 2024
203a0fa
shell: Fully type "topnav"
mvollmer Dec 11, 2024
b9239a0
shell: Check types in "shell-modals"
mvollmer Dec 11, 2024
35a2d1e
shell: Fix two bugs found by TypeScript
mvollmer Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 120 additions & 73 deletions pkg/shell/nav.jsx → pkg/shell/nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,35 @@
/*
* This file is part of Cockpit.
*
* Copyright (C) 2024 Red Hat, Inc.
*
* Cockpit is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation; either version 2.1 of the License, or
* (at your option) any later version.
*
* Cockpit is distributed in the hope that it will be useful, but
* WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with Cockpit; If not, see <https://www.gnu.org/licenses/>.
*/

import cockpit from "cockpit";

import React, { useEffect, useState } from 'react';
import PropTypes from 'prop-types';

import { Button } from "@patternfly/react-core/dist/esm/components/Button/index.js";
import { Nav } from "@patternfly/react-core/dist/esm/components/Nav/index.js";
import { SearchInput } from "@patternfly/react-core/dist/esm/components/SearchInput/index.js";
import { Tooltip, TooltipPosition } from "@patternfly/react-core/dist/esm/components/Tooltip/index.js";
import { ContainerNodeIcon, ExclamationCircleIcon, ExclamationTriangleIcon, InfoCircleIcon } from '@patternfly/react-icons';

import { encode_location } from "./util.jsx";
import { Location, encode_location, ManifestItem } from "./util.jsx";
import { ShellState, PageStatus } from "./state";
import { ManifestKeyword } from "./machines/machines";

const _ = cockpit.gettext;

Expand All @@ -22,8 +42,8 @@ export const SidebarToggle = () => {
* However, when clicking on an iframe moves focus to its content's window that triggers the main window.blur event.
* Additionally, when clicking on an element in the same iframe make sure to unset the 'active' state of the 'System' dropdown selector.
*/
const handleClickOutside = (ev) => {
if (ev.target.id == "nav-system-item")
const handleClickOutside = (ev: Event) => {
martinpitt marked this conversation as resolved.
Show resolved Hide resolved
if ((ev.target as Element).id == "nav-system-item")
martinpitt marked this conversation as resolved.
Show resolved Hide resolved
return;

setActive(false);
Expand All @@ -37,7 +57,7 @@ export const SidebarToggle = () => {
}, []);

useEffect(() => {
document.getElementById("nav-system").classList.toggle("interact", active);
document.getElementById("nav-system")!.classList.toggle("interact", active);
}, [active]);

return (
Expand All @@ -50,8 +70,45 @@ export const SidebarToggle = () => {
);
};

interface NavKeyword {
keyword: string;
score: number;
goto: string | null;
}

interface NavItem extends ManifestItem {
keyword: NavKeyword;
}

interface ItemGroup<T> {
name: string;
items: T[];
action?: {
label: string;
target: Location;
} | undefined;
}

interface CockpitNavProps {
groups: ItemGroup<ManifestItem>[];
selector: string;
current: string;
filtering: (item: ManifestItem, term: string) => NavItem | null;
sorting: (a: NavItem, b: NavItem) => number;
item_render: (item: NavItem, term: string) => React.ReactNode;
jump: (loc: Location) => void;
}

interface CockpitNavState {
search: string;
current: string;
}

export class CockpitNav extends React.Component {
constructor(props) {
props: CockpitNavProps;
state: CockpitNavState;

constructor(props : CockpitNavProps) {
super(props);

this.state = {
Expand All @@ -60,60 +117,59 @@ export class CockpitNav extends React.Component {
};

this.clearSearch = this.clearSearch.bind(this);
this.props = props;
}

componentDidMount() {
const self = this;
const sel = this.props.selector;
// Click on active menu item (when using arrows to navigate through menu)
function clickActiveItem() {
const cur = document.activeElement;
if (cur.nodeName === "INPUT") {
const el = document.querySelector("#" + sel + " li:first-of-type a");
if (cur instanceof HTMLInputElement) {
const el: HTMLElement | null = document.querySelector("#" + sel + " li:first-of-type a");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to stare at this for a while: querySelector is documented to return Element or null, so here we know that a always is indeed a HTMLElement subclass. 👍

But further below you use querySelector<HTMLElement>(). I think we should pick one consistent syntax, and the latter looks a bit nicer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I admit I haven't looked into this deeper. Will do.

if (el)
el.click();
} else {
} else if (cur instanceof HTMLElement) {
cur.click();
}
}

// Move focus to next item in menu (when using arrows to navigate through menu)
// With arguments it is possible to change direction
function focusNextItem(begin, step) {
function focusNextItem(begin: number, step: number) {
const cur = document.activeElement;
const all = Array.from(document.querySelectorAll("#" + sel + " li a"));
if (cur.nodeName === "INPUT" && all) {
const all = Array.from(document.querySelectorAll<HTMLElement>("#" + sel + " li a"));
if (cur instanceof HTMLInputElement && all.length > 0) {
if (begin < 0)
begin = all.length - 1;
all[begin].focus();
} else {
let i = all.findIndex(item => item === cur);
i += step;
if (i < 0 || i >= all.length)
document.querySelector("#" + sel + " .pf-v5-c-text-input-group__text-input").focus();
document.querySelector<HTMLElement>("#" + sel + " .pf-v5-c-text-input-group__text-input")?.focus();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

else
all[i].focus();
}
}

function navigate_apps(ev) {
if (ev.keyCode === 13) // Enter
const navigate_apps = (ev: KeyboardEvent) => {
if (ev.key == "Enter")
clickActiveItem();
else if (ev.keyCode === 40) // Arrow Down
else if (ev.key == "ArrowDown")
focusNextItem(0, 1);
else if (ev.keyCode === 38) // Arrow Up
else if (ev.key == "ArrowUp")
focusNextItem(-1, -1);
else if (ev.keyCode === 27) { // Escape - clean selection
self.setState({ search: "" });
document.querySelector("#" + sel + " .pf-v5-c-text-input-group__text-input").focus();
else if (ev.key == "Escape") {
this.setState({ search: "" });
document.querySelector<HTMLElement>("#" + sel + " .pf-v5-c-text-input-group__text-input")?.focus();
}
}
};

document.getElementById(sel).addEventListener("keyup", navigate_apps);
document.getElementById(sel).addEventListener("change", navigate_apps);
document.getElementById(sel)?.addEventListener("keyup", navigate_apps);
}

static getDerivedStateFromProps(nextProps, prevState) {
static getDerivedStateFromProps(nextProps: CockpitNavProps, prevState: CockpitNavState) {
if (nextProps.current !== prevState.current)
return {
search: "",
Expand All @@ -127,10 +183,10 @@ export class CockpitNav extends React.Component {
}

render() {
const groups = [];
const groups: ItemGroup<NavItem>[] = [];
const term = this.state.search.toLowerCase();
this.props.groups.forEach(g => {
const new_items = g.items.map(i => this.props.filtering(i, term)).filter(Boolean);
const new_items = g.items.map(i => this.props.filtering(i, term)).filter((i): i is NavItem => !!i);
Comment on lines -133 to +189
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What difference does that is make? In the end it gets coerced into a boolean anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes no difference! :-O

I put it there to tell TS that there are no nulls in the result of filtering anymore so that new_items gets type NavItem[].

But, just rewriting this from Boolean to i => !!i is enough to clue in TS. No type guard needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked in Matrix, for posterity: ts doesn't understand that "Boolean(x) == true" implies that "x !== null". That feels like an outright bug to me. so i => !!i makes complete sense. The type guard is indeed lost/unnecesary, so at least I did understand that part right.

new_items.sort(this.props.sorting);
if (new_items.length > 0)
groups.push({ name: g.name, items: new_items, action: g.action });
Expand All @@ -139,7 +195,7 @@ export class CockpitNav extends React.Component {
return (
<>
<SearchInput placeholder={_("Search")} value={this.state.search} onChange={(_, search) => this.setState({ search })} onClear={() => this.setState({ search: "" })} className="search" />
<Nav onSelect={this.onSelect} theme="dark">
<Nav theme="dark">
Comment on lines -142 to +198
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't just a typing change. Separate commit with explanation, or accidental edit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate commit, good point. (There is no "this.onSelect" on CockpitNav.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, tsc told you "no such property, bro!"? Nice!

{ groups.map(g =>
<section className="pf-v5-c-nav__section" aria-labelledby={"section-title-" + g.name} key={g.name}>
<div className="nav-group-heading">
Expand All @@ -148,7 +204,8 @@ export class CockpitNav extends React.Component {
<a className="pf-v5-c-nav__section-title nav-item"
href={encode_location(g.action.target)}
onClick={ ev => {
this.props.jump(g.action.target);
if (g.action)
this.props.jump(g.action.target);
Comment on lines +207 to +208
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test.

ev.preventDefault();
}}>
{g.action.label}
Expand All @@ -168,20 +225,11 @@ export class CockpitNav extends React.Component {
}
}

CockpitNav.propTypes = {
groups: PropTypes.array.isRequired,
selector: PropTypes.string.isRequired,
item_render: PropTypes.func.isRequired,
filtering: PropTypes.func.isRequired,
sorting: PropTypes.func.isRequired,
jump: PropTypes.func.isRequired,
};

function PageStatus({ status, name }) {
function PageStatus({ status, name } : { status: PageStatus, name: string }) {
// Generate name for the status
let desc = name.toLowerCase().split(" ");
desc.push(status.type);
desc = desc.join("-");
const desc_parts = name.toLowerCase().split(" ");
desc_parts.push(status.type);
const desc = desc_parts.join("-");
Comment on lines +230 to +232
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

const desc = [...name.toLowerCase().split(" "), "-"];

(feel free to ignore, bikeshedding)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the same! :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, misread-- [...name.toLowerCase().split(" "), status.type].join("-") clearly!


return (
<Tooltip id={desc + "-tooltip"} content={status.title}
Expand All @@ -197,8 +245,8 @@ function PageStatus({ status, name }) {
);
}

function FormattedText({ keyword, term }) {
function split_text(text, term) {
function FormattedText({ keyword, term } : { keyword: string, term: string }) {
function split_text(text: string, term: string) {
const b = text.toLowerCase().indexOf(term);
const e = b + term.length;
return [text.substring(0, b), text.substring(b, e), text.substring(e, text.length)];
Expand All @@ -210,7 +258,18 @@ function FormattedText({ keyword, term }) {
);
}

export function CockpitNavItem(props) {
export function CockpitNavItem(props : {
name: string;
martinpitt marked this conversation as resolved.
Show resolved Hide resolved
header?: string;
className?: string;
active: boolean;
status: PageStatus | null;
keyword: string;
term: string;
href: string;
onClick: () => void;
actions?: React.ReactNode;
}) {
const s = props.status;
const name_matches = props.keyword === props.name.toLowerCase();
let header_matches = false;
Expand Down Expand Up @@ -243,38 +302,26 @@ export function CockpitNavItem(props) {
);
}

CockpitNavItem.propTypes = {
name: PropTypes.string.isRequired,
href: PropTypes.string.isRequired,
onClick: PropTypes.func,
status: PropTypes.object,
active: PropTypes.bool,
keyword: PropTypes.string,
term: PropTypes.string,
header: PropTypes.string,
actions: PropTypes.node,
};

export const PageNav = ({ state }) => {
export const PageNav = ({ state } : { state: ShellState }) => {
const {
current_machine,
current_manifest_item,
current_machine_manifest_items,
page_status,
} = state;

if (!current_machine || current_machine.state != "connected") {
if (!current_machine || current_machine.state != "connected" || !current_machine_manifest_items || !current_manifest_item) {
Comment on lines -266 to +313
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-typing behaviour change? Or does that just help with type narrowing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for narrowing. This is better written as an assert.

return null;
}

// Filtering of navigation by term
function keyword_filter(item, term) {
function keyword_relevance(current_best, item) {
function keyword_filter(item: ManifestItem, term: string): NavItem | null {
function keyword_relevance(current_best: NavKeyword, item: ManifestKeyword) {
const translate = item.translate || false;
const weight = item.weight || 0;
let score;
let _m = "";
let best = { score: -1 };
let best: NavKeyword = { keyword: "", score: -1, goto: null };
item.matches.forEach(m => {
if (translate)
_m = _(m);
Expand All @@ -293,20 +340,19 @@ export const PageNav = ({ state }) => {
score = 1 + weight;
}
if (score > best.score) {
best = { keyword: m, score };
best = { keyword: m, score, goto: item.goto || null };
}
});
if (best.score > current_best.score) {
current_best = { keyword: best.keyword, score: best.score, goto: item.goto || null };
current_best = best;
}
return current_best;
}

const new_item = Object.assign({}, item);
new_item.keyword = { score: -1 };
const new_item: NavItem = Object.assign({ keyword: { keyword: "", score: -1, goto: null } }, item);
if (!term)
return new_item;
const best_keyword = new_item.keywords.reduce(keyword_relevance, { score: -1 });
const best_keyword = new_item.keywords.reduce(keyword_relevance, { keyword: "", score: -1, goto: null });
if (best_keyword.score > -1) {
new_item.keyword = best_keyword;
return new_item;
Expand All @@ -315,8 +361,8 @@ export const PageNav = ({ state }) => {
}

// Rendering of separate navigation menu items
function nav_item(item, term) {
const active = current_manifest_item.path === item.path;
function nav_item(item: NavItem, term: string) {
const active = current_manifest_item?.path === item.path;

// Parse path
let path = item.path;
Expand All @@ -330,10 +376,10 @@ export const PageNav = ({ state }) => {

// Parse page status
let status = null;
if (page_status[current_machine.key])
status = page_status[current_machine.key][item.path];
if (page_status[current_machine!.key])
status = page_status[current_machine!.key][item.path];

const target_location = { host: current_machine.address, path, hash };
const target_location = { host: current_machine!.address, path, hash };

return (
<CockpitNavItem key={item.label}
Expand All @@ -347,7 +393,7 @@ export const PageNav = ({ state }) => {
);
}

const groups = [
const groups: ItemGroup<ManifestItem>[] = [
{
name: _("Apps"),
items: current_machine_manifest_items.ordered("dashboard"),
Expand All @@ -366,6 +412,7 @@ export const PageNav = ({ state }) => {
target: {
host: current_machine.address,
path: current_machine_manifest_items.items.apps.path,
hash: "/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a test for this. This is about the "Edit" button in the "Applications" section. It only appears when at least one applications is installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also smells like a bug fix which would be better to split out into a new commit.

}
};

Expand Down
Loading