Skip to content

Conversation

@NatLeung96
Copy link
Collaborator

No description provided.

NatLeung96 and others added 9 commits April 9, 2025 10:29
* Replaced base html button with MUI Button

* Implemented disabled features and tidied up style parameters

* Moved static styling to styled API

* Commented out Button styling from tests

* Update tests and snapshots
* Replaced div with MUI Typography

* Added styling props to Typography

* Updated the tests for label

* Updated symbols test

* Removed ThemeProvider and tidied styling parameters

* Updated tokens and fixed rotation

* Updated snapshots

* Removed redundant css file

* Moved static styling to styled API

* Updated tests and snapshots

* Set display to flex

* Updated snapshots

* Updated snapshot
* Replaced Checkbox with MUI Checkbox

* Removed ThemeProvider and added additional styling to FormControlLabel

* Moved static styling to styled API. Not-allowed cursor when widget is disabled. Added font styling.

* Removed padding

* Removed padding and fixed alignments
@NatLeung96 NatLeung96 linked an issue Apr 15, 2025 that may be closed by this pull request
@NatLeung96 NatLeung96 changed the base branch from master to 80-update-widgets-to-use-mui-base April 15, 2025 09:27
Copy link
Collaborator

@abigailalexander abigailalexander left a comment

Choose a reason for hiding this comment

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

The appearance itself is good, no issues! The main thing otherwise is that I don't see any items listed in a dropdown menu. It seems like this component was never updated to Phoebus properly so was already missing some functionality e.g. using the items prop which is now needed.

Edit: actually, it looks like there's several other unrelated widget changes included in this PR such as Label, BoolButton etc. Is there a reason for this or is it a git mistake (e.g. due to the base branch change?)

Comment on lines 124 to 160
props.onChange(
actions[parseFloat(event.currentTarget.value) - displayOffset]
);
props.onChange(actions[parseInt(event.target.value) - displayOffset]);
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason behind this changing from parseFloat to parseInt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've deconstructed and reconstructed this part a few times as I was trying things out. I must have used parseInt instinctively as this is an array index. I can change it back to parseFloat.

): JSX.Element => <Widget baseWidget={SmartMenuButton} {...props} />;

registerWidget(MenuButton, MenuButtonWidgetProps, "menubutton");
registerWidget(MenuButton, MenuButtonComponentProps, "menubutton");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should remain as MenuButtonWidgetProps, which contains the necessary PV info and position etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are quite right

disabled = true;
}
} else {
if (!(fromPv && pvName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused by what this if statement is doing. It looks like if we don't have a PV name and we aren't getting values/actions from a PV, we set the actions as the options (but if actionsFromPV is false this will be called still?) I could be wrong,though!

I think some of the original function was unclear as it currently doesn't include the items prop (as mentioned in another comment), so it might be worth double checking this has the logic we expect.

@NatLeung96
Copy link
Collaborator Author

Ok. I've now completely reworked the logic portion of the component so, hopefully, it is now doing what we want.

* Replaced base react button group with MUI ToggleButtonGroup

* Commented out expect statements checking the styling

* Removed ThemeProvider

* Moved static styling to styled API

* Updated tests

* Removed redundant height and width for ButtonGroup

* Set opacity to 0.6 for hover and selected-hover

* Added borders to buttons and fixed text wrapping

* Updated snapshots

* Deleted redundant CSS file

* Removed CSS import

* Changed button height/width and hover opacity

* Updated snapshots
* Replaced divs with LinearProgress

* Added width and height to parser

* Implemented vertical orientation

* Implements default width and height if not provided

* Fixed width and height checking

* Fixed label and removed redundant css file

* Moved default width and height declarations to function head

* Removed redundant height and width from bob parser

* Corrected percentage calculation

* Added transparency option

* Added check for out of range values
* Replaced base input with MUI TextField

* Updated theme tokens and changed border colours

* Move static styling to styled API

* Added alignment props to parser

* Handle alignment props

* Updated test and created snapshot

* Removed redundant input component files

* Removed redundant reference to component file

* Removed redundant horizontalAlignment and verticalAlignment props from parser

* Added textAlignV as ChoicePropOpt and removed unnecessary switch statements

* Added if-else for vertical alignment

* Changed default background to teal (#80FFFF)

* Removed redundant border parsing

* Changed color of hover and focussed borders to blue

* Added custom border

* Added multiLine to parser

* Changed default blue border colour

* TextField displays existing PV value if available

* Reformatted prop type declaration and disabled cursor when disabled

* Updated tests, snapshots, and type definitions

* Updated slidecontrol test

* Updated input snapshot

* Corrected writePV function call

* Removed redundant CSS file

* Added blur on Enter

* Removed comment

* Got border thicknesses to change on hover and focus

* Changed border width values for hover and focussed

* Updated snapshot

* Reorganised widgetprop definitions and updated test

* Added useEffect to set initial text value when PV connection is established

* Updated pvName variable in slideControl

* Updated slideControl test

* Added multiline functionality
Copy link
Collaborator

@abigailalexander abigailalexander left a comment

Choose a reason for hiding this comment

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

I tested this with both items specified in Phoebus and items from a PV, this is now all functioning as expected. Just one small change to tidy up and remove a property we don't need.

Comment on lines 49 to 66
const MenuButtonComponentProps = {
pvName: StringPropOpt,
foregroundColor: ColorPropOpt,
backgroundColor: ColorPropOpt,
font: FontPropOpt,
enabled: BoolPropOpt,
value: DTypePropOpt,
actions: ActionsPropType,
connected: BoolPropOpt,
onChange: FuncPropOpt,
// opi specific prop
actionsFromPv: BoolPropOpt,
label: StringPropOpt,
// bob specific prop
items: StringArrayPropOpt,
itemsFromPv: BoolPropOpt
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

value, pvName and connected (and I think actions as well) don't need to be specified here. MenuButtonComponentProps is only used when we combine it with PVWidgetPropType (which already contains these properties) to create MenuButtonWidgetProps. This also means you don't need to create the additional DTypePropOpt type in propTypes.ts

@NatLeung96 NatLeung96 merged commit 2f1f8c8 into 80-update-widgets-to-use-mui-base Apr 23, 2025
2 checks passed
@NatLeung96 NatLeung96 deleted the 97-replace-menubutton-with-mui-menuitem branch April 23, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace MenuButton with MUI MenuItem

3 participants