Skip to content

7093 add sort toggle to prevent report filter dropdowns overlapping #7406

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

Merged
merged 8 commits into from
Apr 30, 2025
Merged

7093 add sort toggle to prevent report filter dropdowns overlapping #7406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
89f94a3
add sortToggle form component
aimee-mcneil-melville Apr 16, 2025
da31b8c
use sortToggle in expiring items
aimee-mcneil-melville Apr 16, 2025
acfbdd3
change to flatbutton with styling on selection
aimee-mcneil-melville Apr 17, 2025
2cdc675
translations and more styling
aimee-mcneil-melville Apr 17, 2025
ef440ba
add sort to standard reports schemas
aimee-mcneil-melville Apr 17, 2025
9453b77
fix import
aimee-mcneil-melville Apr 17, 2025
dc97264
fix toggled state
aimee-mcneil-melville Apr 28, 2025
a83ec1a
Merge branch 'develop' of https://github.com/msupply-foundation/open-…
aimee-mcneil-melville Apr 28, 2025
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
3 changes: 3 additions & 0 deletions client/packages/programs/src/JsonForms/common/JsonForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { materialRenderers } from '@jsonforms/material-renderers';
import {
BooleanField,
booleanTester,
SortToggle,
SortToggleTester,
stringTester,
TextField,
selectTester,
Expand Down Expand Up @@ -204,6 +206,7 @@ const FormComponent = ({

const renderers = [
{ tester: booleanTester, renderer: BooleanField },
{ tester: SortToggleTester, renderer: SortToggle },
{ tester: stringTester, renderer: TextField },
{ tester: numberTester, renderer: NumberField },
{ tester: selectTester, renderer: Selector },
Expand Down
70 changes: 70 additions & 0 deletions client/packages/programs/src/JsonForms/common/components/SortToggle.tsx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. If I could request thinking about one thing, it's that this is pretty hard coded to the sorting use case. A more generic and flexible component could probably be made of this (perhaps in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing that up, I should have covered that in the PR!
I did consider this when creating it, and went with the most straight forward solution to fix the issue. With sorting direction only having two possible options, and without another use case in mind, it made sense to me to hardcode for this case

There is an existing forms pattern with 'options' like in the 'sort-by' dropdown below - to make that pattern work for a flexible toggle button setup it would need at least:

  • logic in the toggle component to get the icon and render to correct icon/label pairing
  • logic to get and set the 'value' as it is not built in to the FlatButton
  • considerations for control over the number of options that could theoretically be added and/or new logic plus styling if able to add more than two (more like a multi-select)

open-msupply/standard_reports/item-usage/latest/argument_schemas/arguments_ui.json

Lines 31 to 46 in 35917ab

"options": {
"show": [
["name", "T#report.item-name"],
["code", "T#label.code"],
["SOH", "T#report.stock-on-hand"],
["MOS", "T#report.months-cover"],
["monthConsumption", "T#report.consumption-month"],
["lastMonthConsumption", "T#report.consumption-last-month"],
["twoMonthsAgoConsumption", "T#report.consumption-two-months-ago"],
["expiringInSixMonths", "T#report.expiring-6-months"],
["expiringInTwelveMonths", "T#report.expiring-12-months"],
["stockOnOrder", "T#report.stock-on-order"],
["AMC12", "T#report.amc-12-months"],
["AMC24", "T#report.amc-24-months"]
]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea with 1-3 short options (especially if icons and no text) toggle is good, <=~5 radio buttons (especially if text) and beyond that is combobox.

Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import React, { useState } from 'react';
import { rankWith, ControlProps, uiTypeIs } from '@jsonforms/core';
import { withJsonFormsControlProps } from '@jsonforms/react';
import {
alpha,
DetailInputWithLabelRow,
Theme,
useTranslation,
} from '@openmsupply-client/common';
import { FORM_LABEL_WIDTH, DefaultFormRowSx } from '../styleConstants';
import { FlatButton } from '@common/components';
import { SortAscIcon, SortDescIcon } from '@common/icons';

export const SortToggleTester = rankWith(10, uiTypeIs('SortToggle'));

type SortDirection = 'asc' | 'desc' | null;

const UIComponent = (props: ControlProps) => {
const { handleChange, label, path, enabled } = props;
const t = useTranslation();
const [sortDirection, setSortDirection] = useState<SortDirection>();

if (!props.visible) {
return null;
}

const selectedStyles = (theme: Theme) => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

custom styling for when the button is the selected option

fontWeight: 'bold',
backgroundColor: alpha(theme.palette.primary.main, 0.3),
'&:hover': {
backgroundColor: alpha(theme.palette.primary.main, 0.3),
},
});

const getSelectedSx = (value: SortDirection) =>
sortDirection === value ? selectedStyles : undefined;

const handleClick = (value: SortDirection) => {
const newValue = sortDirection === value ? null : value;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line with newValue ensures that only one button can be selected at a time - deselecting the button if it’s already selected, or selecting it if it’s not. This is what the MUI ToggleButton propexclusive achieves, whilst using common components we already have

setSortDirection(newValue);
handleChange(path, newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

oh hahahaha ya that'd do it

};

return (
<DetailInputWithLabelRow
label={label}
DisabledInput={!enabled}
labelWidthPercentage={FORM_LABEL_WIDTH}
sx={DefaultFormRowSx}
Input={
<>
<FlatButton
label={t('report.ascending')}
onClick={() => handleClick('asc')}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing in 'asc' as the value, as FlatButton does not have a value attribute. Works in this scenario where only two options exist

startIcon={<SortAscIcon />}
sx={[getSelectedSx('asc') || {}, { borderRadius: 24 }]}
/>
<FlatButton
label={t('report.descending')}
onClick={() => handleClick('desc')}
startIcon={<SortDescIcon />}
sx={[getSelectedSx('desc') || {}, { borderRadius: 24 }]}
/>
</>
}
/>
);
};

export const SortToggle = withJsonFormsControlProps(UIComponent);
1 change: 1 addition & 0 deletions client/packages/programs/src/JsonForms/common/components/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ export * from './Autocomplete';
export * from './ConditionalSelect';
export * from './Spacer';
export * from './ToolbarLayout';
export * from './SortToggle';
4 changes: 2 additions & 2 deletions standard_reports/expiring-items/latest/argument_schemas/arguments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
},
"dir": {
"description": "sort by dir",
"type": "string",
"enum": ["asc", "desc"]
"type": ["string", "null"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included 'null' here to prevent console errors when deselecting

"format": "SortToggle"
},
"monthlyConsumptionLookBackPeriod": {
"description": "Average Monthly Consumption Look Back Period",
Expand Down
10 changes: 2 additions & 8 deletions standard_reports/expiring-items/latest/argument_schemas/arguments_ui.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,9 @@
}
},
{
"type": "Control",
"type": "SortToggle",
"scope": "#/properties/dir",
"label": "T#report.sort-direction",
"options": {
"show": [
["asc", "T#report.ascending"],
["desc", "T#report.descending"]
]
}
"label": "T#report.sort-direction"
}
]
}
4 changes: 2 additions & 2 deletions standard_reports/item-usage/latest/argument_schemas/arguments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
},
"dir": {
Copy link
Contributor

Choose a reason for hiding this comment

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

tangentially I find it funny that we shorten this to dir, meanwhile having properties like "monthlyConsumptionLookBackPeriod". Also just curious is the "description" just for documenting in this JSON or is it shown somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is quite a difference between the property names! It's not immediately obvious (at least to me) what 'dir' represents. I'd be happy to update that to 'direction'
The description is used for internal/documentation only atm, though it looks like another use case is for a Tooltip

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly the reason I even commented is it took me a second too - I was like wait directory?

Nah maybe a refactor issue if anything heh.

Yea I thought description might be a tool tip, but then it'd need localisation! Maybe it should be regardless, but the use case isn't really obvious heh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Chris, I have raised in #7546

"description": "sort by dir",
"type": "string",
"enum": ["asc", "desc"]
"type": ["string", "null"],
"format": "SortToggle"
}
}
}
Expand Down
10 changes: 2 additions & 8 deletions standard_reports/item-usage/latest/argument_schemas/arguments_ui.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,9 @@
}
},
{
"type": "Control",
"type": "SortToggle",
"scope": "#/properties/dir",
"label": "T#report.sort-direction",
"options": {
"show": [
["asc", "T#report.ascending"],
["desc", "T#report.descending"]
]
}
"label": "T#report.sort-direction"
}
]
}
4 changes: 2 additions & 2 deletions standard_reports/stock-detail/latest/argument_schemas/arguments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
},
"dir": {
"description": "sort by dir",
"type": "string",
"enum": ["asc", "desc"]
"type": ["string", "null"],
"format": "SortToggle"
}
}
}
Expand Down
10 changes: 2 additions & 8 deletions standard_reports/stock-detail/latest/argument_schemas/arguments_ui.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,9 @@
}
},
{
"type": "Control",
"type": "SortToggle",
"scope": "#/properties/dir",
"label": "T#report.sort-direction",
"options": {
"show": [
["asc", "T#report.ascending"],
["desc", "T#report.descending"]
]
}
"label": "T#report.sort-direction"
}
]
}
4 changes: 2 additions & 2 deletions standard_reports/stock-status/latest/argument_schemas/arguments.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@
},
"dir": {
"description": "sort by dir",
"type": "string",
"enum": ["asc", "desc"]
"type": ["string", "null"],
"format": "SortToggle"
}
}
}
Expand Down
10 changes: 2 additions & 8 deletions standard_reports/stock-status/latest/argument_schemas/arguments_ui.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,9 @@
}
},
{
"type": "Control",
"type": "SortToggle",
"scope": "#/properties/dir",
"label": "T#report.sort-direction",
"options": {
"show": [
["asc", "T#report.ascending"],
["desc", "T#report.descending"]
]
}
"label": "T#report.sort-direction"
}
]
}
Loading