Skip to content

Commit

Permalink
chore: number input
Browse files Browse the repository at this point in the history
The current number editor in Settings > Preferences works, but it has a
few drawbacks:
 - doesn't match current design, and would need updating to light mode.
 - periodically has some odd 'resets' to a previous number. (this isn't
   just the input - figured out later this is #7142)
 - has two error modes - underline for below minimum, icon to the left
   for invalid value.
 - after invalid entry (e.g. below minimum), incrementing does a string
   add vs numerical.
 - not easily reusable for editing numbers in other places.

This creates a NumberInput component that is just an Input with +/- buttons.
Key input is restricted to numbers, and increment/decrement are used to
disable +/- appropriately. If the user manually enters a number out of
bounds, the Input's error is used to display a short warning.

Input required some minor changes to support this:
 - allow inputClass in order to center numbers in the textbox.
 - passthrough of key events, in order to block non-numerical entry.
 - boolean to block showing the error in a separate span. Although this is
   the design for inputs, it felt really out of place in preferences.
 - reduce padding slightly - it didn't matter in large Inputs, but
   number inputs tend to be smaller and extraneous padding was obvious.

New component is used in Settings > Preferences and max retry count for
running an image.

Tests added. NumberItem test had to be changed since only numerical inputs
work now.

Another part of #3234.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
  • Loading branch information
deboer-tim committed May 10, 2024
1 parent 652bb8d commit 6a36ee4
Show file tree
Hide file tree
Showing 7 changed files with 275 additions and 140 deletions.
9 changes: 4 additions & 5 deletions packages/renderer/src/lib/image/RunImage.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { ContainerInfoUI } from '../container/ContainerInfoUI';
import { splitSpacesHandlingDoubleQuotes } from '../string/string';
import ErrorMessage from '../ui/ErrorMessage.svelte';
import FormPage from '../ui/FormPage.svelte';
import NumberInput from '../ui/NumberInput.svelte';
import Tab from '../ui/Tab.svelte';
import type { ImageInfoUI } from './ImageInfoUI';
Expand Down Expand Up @@ -847,12 +848,10 @@ async function assertAllPortAreValid(): Promise<void> {
<span
class="text-sm w-28 inline-block align-middle whitespace-nowrap text-gray-700"
title="Number of times to retry before giving up.">Retries:</span>
<input
type="number"
min="0"
<NumberInput
minimum="{0}"
bind:value="{restartPolicyMaxRetryCount}"
placeholder="Number of times to retry before giving up"
class="w-full p-2 outline-none text-sm bg-charcoal-800 rounded-sm text-gray-700 placeholder-gray-700"
class="w-full p-2"
disabled="{restartPolicyName !== 'on-failure'}" />
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,28 @@ import { expect, test } from 'vitest';
import type { IConfigurationPropertyRecordedSchema } from '../../../../../main/src/plugin/configuration-registry';
import NumberItem from './NumberItem.svelte';

test('Expect tooltip if value input is NaN', async () => {
test('Expect tooltip if value input is invalid', async () => {
const record: IConfigurationPropertyRecordedSchema = {
id: 'record',
title: 'record',
parentId: 'parent.record',
description: 'record-description',
type: 'number',
minimum: 1,
minimum: 10,
maximum: 34,
};
const value = 2;
const value = 12;
render(NumberItem, { record, value });

const input = screen.getByLabelText('record-description');
expect(input).toBeInTheDocument();
await userEvent.click(input);
await userEvent.clear(input);
await userEvent.keyboard('unknown');
await userEvent.keyboard('5');

const tooltip = screen.getByLabelText('tooltip');
expect(tooltip).toBeInTheDocument();
expect(tooltip.textContent).toContain('Expecting a number');
expect(tooltip.textContent).toContain('The value cannot be less than 10');
});

test('Expect decrement button disabled if value is less than minimum', async () => {
Expand Down
153 changes: 29 additions & 124 deletions packages/renderer/src/lib/preferences/item-formats/NumberItem.svelte
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
<script lang="ts">
import { Tooltip } from '@podman-desktop/ui-svelte';
import { onMount } from 'svelte';
import type { IConfigurationPropertyRecordedSchema } from '../../../../../main/src/plugin/configuration-registry';
import { uncertainStringToNumber } from '../Util';
import { checkNumericValueValid } from './NumberItemUtils';
import NumberInput from '../../ui/NumberInput.svelte';
export let record: IConfigurationPropertyRecordedSchema;
export let value: number | undefined;
Expand All @@ -13,132 +10,40 @@ export let invalidRecord = (_error: string) => {};
let valueUpdateTimeout: NodeJS.Timeout;
let recordValue: number;
$: recordValue = value || 0;
let numberInputErrorMessage = '';
let numberInputInvalid = false;
let recordValue: number = value || 0;
let lastValue: number;
let error: string | undefined = undefined;
onMount(() => {
if (value && assertNumericValueIsValid(value)) {
recordValue = value;
}
});
$: recordValue = value || 0;
function onInput(event: Event) {
// clear the timeout so if there was an old call to onChange pending is deleted. We will create a new one soon
clearTimeout(valueUpdateTimeout);
const target = event.currentTarget as HTMLInputElement;
// convert string to number
const _value: number = uncertainStringToNumber(target.value);
// if number is not valid (NaN), return
if (!assertNumericValueIsValid(_value)) {
invalidRecord(numberInputErrorMessage);
return;
}
recordValue = _value;
$: if (recordValue) {
const newValue = Number(recordValue);
// if the value is different from the original update
if (record.id && recordValue !== value) {
valueUpdateTimeout = setTimeout(() => onChange(record.id!, recordValue), 500);
}
}
function onNumberInputKeyPress(event: any) {
// if the key is not a number skip it
if (isNaN(Number(event.key))) {
event.preventDefault();
}
}
function onDecrement(
e: MouseEvent & {
currentTarget: EventTarget & HTMLButtonElement;
},
) {
// clear the timeout so if there was an old call to onChange pending is deleted. We will create a new one soon
clearTimeout(valueUpdateTimeout);
// if we can decrement
if (record.id && canDecrement(recordValue)) {
// update record
recordValue -= 1;
// verify it is valid
// it may happen that the value is greater than min but also greater than max so we need to check if we can update it
if (assertNumericValueIsValid(recordValue)) {
valueUpdateTimeout = setTimeout(() => onChange(record.id!, recordValue), 500);
}
}
e.preventDefault();
}
function onIncrement(
e: MouseEvent & {
currentTarget: EventTarget & HTMLButtonElement;
},
) {
// clear the timeout so if there was an old call to onChange pending is deleted. We will create a new one soon
clearTimeout(valueUpdateTimeout);
// if we can increment
if (record.id && canIncrement(recordValue)) {
// update record
recordValue += 1;
// verify it is valid
// it may happen that the value is less than max but also less than min so we need to check if we can update it
if (assertNumericValueIsValid(recordValue)) {
valueUpdateTimeout = setTimeout(() => onChange(record.id!, recordValue), 500);
}
if (record.id && newValue !== lastValue && !error) {
// clear the timeout so if there was an old call to onChange pending is deleted. We will create a new one soon
clearTimeout(valueUpdateTimeout);
valueUpdateTimeout = setTimeout(() => {
onChange(record.id!, newValue);
lastValue = newValue;
}, 500);
}
e.preventDefault();
}
function canDecrement(value: number) {
return !record.minimum || value > record.minimum;
}
function assertNumericValueIsValid(value: number): boolean {
const numericValue = checkNumericValueValid(record, value);
numberInputInvalid = !numericValue.valid;
numberInputErrorMessage = numericValue.error || '';
return numericValue.valid;
}
function canIncrement(value: number) {
return !record.maximum || (typeof record.maximum === 'number' && value < record.maximum);
$: if (error) {
invalidRecord(error);
}
</script>

<div
class="flex flex-row rounded-sm bg-zinc-700 text-sm divide-x divide-charcoal-800 w-24 border-b"
class:border-violet-500="{!numberInputInvalid}"
class:border-red-500="{numberInputInvalid}">
<button
data-action="decrement"
aria-label="decrement"
on:click="{onDecrement}"
disabled="{!canDecrement(recordValue)}"
class="w-11 text-white {!canDecrement(recordValue)
? 'bg-charcoal-600 text-charcoal-100 border-t border-l border-charcoal-800'
: 'hover:text-gray-900 hover:bg-gray-700'} cursor-pointer outline-none">
<span class="m-auto font-thin">−</span>
</button>
<Tooltip topLeft tip="{numberInputErrorMessage}">
<input
type="text"
class="w-[50px] outline-none focus:outline-none text-white text-center text-sm py-0.5 bg-transparent"
name="{record.id}"
bind:value="{recordValue}"
on:keypress="{event => onNumberInputKeyPress(event)}"
on:input="{onInput}"
aria-label="{record.description}" />
</Tooltip>
<button
data-action="increment"
aria-label="increment"
on:click="{onIncrement}"
disabled="{!canIncrement(recordValue)}"
class="w-11 text-white {!canIncrement(recordValue)
? 'bg-charcoal-600 text-charcoal-100 border-t border-l border-charcoal-800'
: 'hover:text-gray-900 hover:bg-gray-700'} cursor-pointer outline-none">
<span class="m-auto font-thin">+</span>
</button>
</div>
<Tooltip topLeft tip="{error}">
<NumberInput
class="w-24"
name="{record.id}"
bind:value="{recordValue}"
bind:error="{error}"
aria-label="{record.description}"
minimum="{record.minimum}"
maximum="{record.maximum && typeof record.maximum === 'number' ? record.maximum : undefined}"
showError="{false}">
</NumberInput>
</Tooltip>
135 changes: 135 additions & 0 deletions packages/renderer/src/lib/ui/NumberInput.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/**********************************************************************
* Copyright (C) 2024 Red Hat, Inc.
*
* Licensed 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.
*
* SPDX-License-Identifier: Apache-2.0
***********************************************************************/

/* eslint-disable @typescript-eslint/no-explicit-any */

import '@testing-library/jest-dom/vitest';

import { render, screen } from '@testing-library/svelte';
import userEvent from '@testing-library/user-event';
import { expect, test } from 'vitest';

import NumberInput from './NumberInput.svelte';

function renderInput(name: string, value: number, disabled?: boolean, minimum?: number, maximum?: number): void {
render(NumberInput, {
name: name,
value: value,
disabled: disabled,
minimum: minimum,
maximum: maximum,
});
}

test('Expect basic styling', async () => {
renderInput('test', 5);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();

const decrement = screen.getByLabelText('decrement');
expect(decrement).toBeInTheDocument();
expect(decrement).toHaveClass('pr-0.5');
expect(decrement).toHaveClass('text-[var(--pd-input-field-stroke)]');
expect(decrement).toHaveClass('group-hover:text-[var(--pd-input-field-hover-stroke)]');

const increment = screen.getByLabelText('increment');
expect(increment).toBeInTheDocument();
expect(increment).toHaveClass('pl-0.5');
expect(increment).toHaveClass('text-[var(--pd-input-field-stroke)]');
expect(increment).toHaveClass('group-hover:text-[var(--pd-input-field-hover-stroke)]');
});

test('Expect disabled styling', async () => {
renderInput('test', 5, true);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();

const decrement = screen.getByLabelText('decrement');
expect(decrement).toBeInTheDocument();
expect(decrement).toHaveClass('pr-0.5');
expect(decrement).toHaveClass('text-[var(--pd-input-field-disabled-text)]');

const increment = screen.getByLabelText('increment');
expect(increment).toBeInTheDocument();
expect(increment).toHaveClass('pl-0.5');
expect(increment).toHaveClass('text-[var(--pd-input-field-disabled-text)]');
});

test('Expect decrement works', async () => {
renderInput('test', 5);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();
expect(input).toHaveValue('5');

const decrement = screen.getByLabelText('decrement');
expect(decrement).toBeInTheDocument();

await userEvent.click(decrement);

expect(input).toHaveValue('4');
});

test('Expect minimum value works', async () => {
renderInput('test', 11, false, 10, 100);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();
expect(input).toHaveValue('11');

const decrement = screen.getByLabelText('decrement');
expect(decrement).toBeInTheDocument();
expect(decrement).toBeEnabled();

await userEvent.click(decrement);

expect(decrement).toBeDisabled();
});

test('Expect increment works', async () => {
renderInput('test', 5);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();
expect(input).toHaveValue('5');

const increment = screen.getByLabelText('increment');
expect(increment).toBeInTheDocument();

await userEvent.click(increment);

expect(input).toHaveValue('6');
});

test('Expect maximum value works', async () => {
renderInput('test', 99, false, 10, 100);

const input = screen.getByRole('textbox');
expect(input).toBeInTheDocument();
expect(input).toHaveValue('99');

const increment = screen.getByLabelText('increment');
expect(increment).toBeInTheDocument();
expect(increment).toBeEnabled();

await userEvent.click(increment);

expect(increment).toBeDisabled();
});

0 comments on commit 6a36ee4

Please sign in to comment.