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.

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

Update widget props #44

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Update widget props #44

merged 8 commits into from
Jan 23, 2024

Conversation

abigailalexander
Copy link
Collaborator

@abigailalexander abigailalexander commented Jan 18, 2024

Updated the following widget properties:

  • Changed Ellipse gradient. It now goes in the same direction as CSS/Phoebus when horizontalFill is set to true/false. Also uses backgroundColor and bgGradientColor for the gradient.
  • bgGradientColor and fgGradientColor now use properties bg_gradient_color and fg_gradient_color. Previously they mistakenly used properties background_color and foreground_color, and this caused issues with rules for changing background colours in the Shape widget.
  • Added object-fit property to Image, so when stretchToFit is true the image fills the full space of the div and does not maintain aspect ratio. This is the same behaviour seen in CSS/Phoebus
  • Added the cornerHeight and cornerWidth properties to Shape widget, allowing for rounded corners on rectangles.

I also updated tests and snapshots affected by the changes to widgets.

@abigailalexander
Copy link
Collaborator Author

The latest commit adds the scroll property to the DynamicPage widget - this is used to set whether the .opi file opened hides overflow or adds a scroll bar. This functionality already exists in EmbeddedDisplay but because it did not exist in DynamicPage, any screens opened through clicking an ActionButton didn't have it passed in.

@abigailalexander
Copy link
Collaborator Author

Also, now added RoundedRectangle to list of widgets parsed. This is identical to the Rectangle widget except for the corner_width and corner_height properties, so is parsed into the same Shape widget.

@rjwills28
Copy link
Collaborator

Thanks for the changes. I've tested the different widgets and here are some comments:

  • Rectangle corner_width and corner_height work nicely. These can be used by adding the properties to a normal 'Rectangle' shape (as is done in Phoebus) and with a 'Rounded Rectangle' shape in CSS.
  • The horizontalFill trigger true/false now works in the same way as CSS.
  • I did notice a difference in behaviour between CSS and the web when using a background gradient. See example below where I have blue as the background colour and orange as the gradient colour.
    ellipse_css ellipse_web
    It looks like CSS brings the gradient colour in at the top while the web is bring it in at the bottom?
  • I verified that the stretchToFit property works as it does in CSS when the image size is the same or as large as the actual image, i.e. it will stretch to fill the space and not maintain aspect ratio. However the behaviour looks slightly different when you shrink an image so that the div is smaller than the image size. In this case, if stretchToFit is no, then it does not try to shrink the image to the size of the div and instead just shows a smaller portion of the image. In the example below you can see that CSS shows the top-left corner of the DLS building, while the web shows some portion in the middle.
    image_css
    image_web
    I'm not sure this difference is due to changes in this PR but something worth noting. Also this feature may not be used like this as really people should use the crop features of the image widget if they only want to show a portion of it.

@rjwills28
Copy link
Collaborator

I've also tested the scroll property on a dynamicpage and this now works as expected.
One small thing I noticed, which is not related to this PR but I'll still mention in case we want to fix it in another issue, is that in json file the scroll and executeAsOne properties take a boolean value, whereas showCloseButton takes a boolean wrapped as a string, i.e.

"scroll": true,
"executeAsOne": false,
"showCloseButton": "true"

I think these should be consistent as it is easy to make a mistake and use the wrong type when creating the json file (I did first off). Presumably showCloseButton should just be a boolean? I can open a new issue if we think this is worth updating.

@abigailalexander
Copy link
Collaborator Author

I've updated the Ellipse gradient so it should now mimic the CSS behaviour. The stretchToFit Image behaviour is very odd, and might be worth its own issue if we think that someone might be using it in that way. I can't see anything immediate in the web code that suggests why it centers the image, or why CSS uses the edge.

I also think it's worth changing showCloseButton to be a boolean as well

Copy link
Collaborator

@rjwills28 rjwills28 left a comment

Choose a reason for hiding this comment

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

Perfect. thanks for the changes. I'll create an issue for both of the remaining items.
The actual code and test updates look good to me.

@abigailalexander abigailalexander merged commit 9c09c49 into master Jan 23, 2024
2 checks passed
@abigailalexander abigailalexander deleted the update-widget-props branch September 25, 2024 12:47
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.

2 participants