Skip to content

Conversation

@mikeharv
Copy link
Contributor

The basics

The details

Resolves

Fixes #2351 (review)

Proposed Changes

  1. Override maximum-height rule for the bitmap field dropdown's editor, which will allow large images to be displayed without scrolling.
  2. Conditionally add another class if the editor has buttons, and only then use an extra 20px bottom margin.

Reason for Changes

While testing a recent update to the bitmap field plugin (#2351), it was noticed that dropdown content is cut off for large images.

In local testing with the supplied test blocks, however, I have noted what appears to be a bug with sizing the pop-up for the "Static field height, custom colors, no buttons" test block:

331197691-3a1423e4-43fc-4ac8-aa72-db393d4746c6

This bug is unrelated to the recent update, and is due instead to a max height set in Core for all Blockly dropdowns: https://github.com/google/blockly/blob/049993405e23cf4bf05943eb0d5070116715db2c/core/css.ts#L127

While doing this, I also noticed an existing 20px bottom margin, which seemed undesirable if there were no buttons:
imageimage

Because buttons were always present before the most recent update, this bug is directly related to that work.

The results of the changes here are that the editor no longer has a maximum height, and there is equal padding on all sides when no buttons are shown:
image

The obvious drawback to proposed solution is that block designers will need to be especially careful not to provide blocks with images that are overly large for their users' devices and screen resolution. Alternative solutions could be to dynamically resize the editor content or to keep a (potentially adjusted) maximum height and live with the scroll bar. I didn't pursue either of those options at this time.

Test Coverage

No changes to test coverage are included.

Documentation

No changes to documentation are needed.

Additional Information

N/A

@mikeharv mikeharv requested a review from a team as a code owner May 16, 2024 15:48
@mikeharv mikeharv requested review from maribethb and removed request for a team May 16, 2024 15:48
margin: 5px 0;
}
.blocklyDropDownContent{
max-height: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately adding this css will break unrelated dropdowns, e.g. you can get this with the test toolbox:
Screenshot 2024-05-16 at 5 01 32 PM
where the dropdown is so long it extends past the workspace.

I think you probably want to unset it just for dropdowns that contain the bitmap field, not for all dropdowns just because this plugin is loaded. However, that is easier said than done. There's a pseudo-selector you can use for this but it's pretty new / still a working draft https://caniuse.com/css-has

While you can add an additional class to the dropdownDiv (or add an inline style) when you show the editor, there's no corresponding method that gets called when the field's editor is hidden, so there's no opportunity to remove it. So I can't actually think of a solution to this right now :/

I would remove this from this PR and you mentioned some other ideas you had for fixing the height you could follow up with. You could also just warn users in the readme not to set the height greater than the max-height of dropdowns (which by default is 300px)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, good catch! It looks like we can potentially handle this by adding the additional class during dropdownCreate and removing it during dropdownDispose. See newest commit. This seems to work as expected when switching between two different dropdown field types' editors. Thoughts?

@maribethb maribethb merged commit 4cc25b3 into RaspberryPiFoundation:master May 20, 2024
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