84 replace action button with mui button

[NatLeung96]

I've changed the ActionButtonComponent so that it returns a MUI Button instead of a html button.

[abigailalexander]

Overall this looks good, but there are a couple of cases where the behaviour isn't as expected. I've mentioned most in comments, but to summarise the big ones:

• The default colours used in your theme are different to Phoebus
• Text overflow and wrapping isn't handled
• If you set your button size down to 20x20 pixels, you'll notice it doesn't shrink small enough. This is because MUI has a minWidth and minHeight property, and also applies padding on top of the width you've specified (you can see this also by inspecting the component and comparing the size of the top level div with the MUI button). Our 20x20 component is now 64x24 in size.

I attached some pictures below to show the difference. The left one is how it should look.
(You'll notice the font also doesn't rotate on the transparent button. This is an existing bug, so if you want you can fix it in this PR, or we'll open an issue for it to be fixed at a separate time)

[abigailalexander]

Removing this commonCss function also removes the ability to set the button backgroundColour as transparent and some other CSS. It would be good to either keep using this function, or ensure we're still setting all the needed CSS.

[NatLeung96]

I've made it so that the transparent field from the .bob file actually gets passed to the widget and added a test to make the background transparent if it is set to true. The way it currently works is that, when transparent, the button colour matches the background but it keeps the button shadow. Personally, I think this looks cool but I can remove it if desired.

[abigailalexander]

Thanks for adding that missing prop in! I think it might look better if we change the button variant if the background colour is transparent from contained (which has the shadow) to text. This ends up looking closest to Phoebus appearance

[NatLeung96]

There is still an issue with the text rotation. I'm not 100% sure if it is a parser problem or a case of the rotationStep variable not being passed to the widget properly. I tried adding a "rotation_step" token to the simple parser dictionary but, in testing, it returns undefined instead of a number.

Regardless, I've added an implementation that should rotate the text on the assumption that a rotationStep is passed.

[abigailalexander]

Overall looks a lot closer to Phoebus now, thanks for doing those changes! I noticed the the very small button (20x20 px) still doesn't size right and I think this is due to the lineHeight property. This is scaled by the font and dictates the minimum height of a component based on the height of the text. I'd try setting this property to "unset" and check if it works fine

Also, this isn't a necessary change but might be good for code organisation: MUI has a styled functionality that is useful for static style modification as we can redefine the style of the base component. It just makes everything a bit tidier and easier to read, and I'd suggest using it for things like "&.MuiButton-root" properties that are static and not affected by the props. e.g.

// Rename the import so we don't end up duplicate component names after styling
import { Button as MuiButton } from "@mui/material";

const Button = styled(MuiButton)({
   // Put all the static styling here
    display: "block",
    "&.MuiButton-root": {
        minWidth: 0
        etc...
    }
}

export const ActionButtonComponent = (
  props: ActionButtonProps
): JSX.Element => {
  // All the code here
   ...
   return (
     <Button //This has the static styling set, now we define dynamic styling
        sx={color: foregroundColor.toString(), ...}
     > 
     ...
  }

Not every component will need dedicated styling, but if we're setting a lot of properties it might be good to use.

Lastly, for rotationStep to be passed as a prop it needs to be added to the parser list. This can be found in src/ui/widgets/EmbeddedDisplay/bobParser.ts in the parseBob function. If you add it to the list of properties there, it should hopefully work :)

[abigailalexander]

Thanks for adding that missing prop in! I think it might look better if we change the button variant if the background colour is transparent from contained (which has the shadow) to text. This ends up looking closest to Phoebus appearance

[NatLeung96]

Overall looks a lot closer to Phoebus now, thanks for doing those changes! I noticed the the very small button (20x20 px) still doesn't size right and I think this is due to the lineHeight property. This is scaled by the font and dictates the minimum height of a component based on the height of the text. I'd try setting this property to "unset" and check if it works fine

"unset" doesn't seem to do anything. Apparently, the default behaviour is dependent on the renderer. Instead, I set it manually to 1 (i.e. equal to the height of the font).

Also, this isn't a necessary change but might be good for code organisation: MUI has a styled functionality that is useful for static style modification as we can redefine the style of the base component. It just makes everything a bit tidier and easier to read, and I'd suggest using it for things like "&.MuiButton-root" properties that are static and not affected by the props. e.g.

Fair. I do agree that it looks a bit more organised.

Lastly, for rotationStep to be passed as a prop it needs to be added to the parser list. This can be found in src/ui/widgets/EmbeddedDisplay/bobParser.ts in the parseBob function. If you add it to the list of properties there, it should hopefully work :)

I've added "rotation_step" to the simpleParser dict, which is what I tried previously (I didn't appear to save it to the commits). This doesn't work the way that I have done it.

[abigailalexander]

Looks good to me!