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

fix(alerts/reports): implementing custom_width as an Antd number input #27260

Merged
merged 3 commits into from
Mar 15, 2024
Merged

fix(alerts/reports): implementing custom_width as an Antd number input #27260

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
823e2d2
changing custom_width input to a NumberInput
fisjac Feb 26, 2024
7a58217
types
fisjac Feb 26, 2024
3305aba
TS fix
fisjac Mar 13, 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
14 changes: 10 additions & 4 deletions superset-frontend/src/features/alerts/AlertReportModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
import rison from 'rison';
import { useSingleViewResource } from 'src/views/CRUD/hooks';

import { Input } from 'src/components/Input';
import { InputNumber } from 'src/components/Input';
import { Switch } from 'src/components/Switch';
import Modal from 'src/components/Modal';
import Collapse from 'src/components/Collapse';
Expand Down Expand Up @@ -873,6 +873,10 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
updateAlertState(name, parsedValue);
};

const onCustomWidthChange = (value: number | null | undefined) => {
updateAlertState('custom_width', value);
};

const onTimeoutVerifyChange = (
event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
) => {
Expand Down Expand Up @@ -1542,12 +1546,14 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
>
<div className="control-label">{t('Screenshot width')}</div>
<div className="input-container">
<Input
<InputNumber
type="number"
name="custom_width"
value={currentAlert?.custom_width || ''}
value={currentAlert?.custom_width ?? null}
Copy link
Member

@michael-s-molina michael-s-molina Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Given that we're setting 600 as the minimum value, should we accept null values here or always start with 600?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have 600 as a default value here. Perhaps we could export in a const for better visibility

Copy link
Contributor Author

@fisjac fisjac Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On the backend, custom_width is a nullable field, and when null, it defaults to either the dashboard or slice attributes of WEBDRIVER_WINDOW in config.py

Default value is:

 WEBDRIVER_WINDOW = {
    "dashboard": (1600, 2000),
    "slice": (3000, 1200),
    "pixel_density": 1,
}

Copy link
Member

@eschutho eschutho Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@fisjac for the ts error here, I think you're going to want the default to be undefined instead of null
error TS2322: Type 'number | null' is not assignable to type 'number | undefined'.

Copy link
Member

Choose a reason for hiding this comment

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

This will never be below 600 though right and it won't ever be null or undefined for that reason too?

min={600}
max={2400}
placeholder={t('Input custom width in pixels')}
onChange={onInputChange}
onChange={onCustomWidthChange}
/>
</div>
</StyledInputContainer>
Expand Down
Loading