repl: Fix image scaling#48435
Conversation
cla-bot
bot
added
the
cla-signed
|
This ensures that the image stays in frame -- the other method creates a crop that is not wanted. |
rgbkrk
left a comment
rgbkrk
left a comment
There was a problem hiding this comment.
Sorry I left these comments and didn't finish submitting the whole review. The only strange bit to me is the removal of the output max width columns setting.
| let image = self.image.clone(); | ||
|
|
||
| div().h(height).w(width).child(img(image)) | ||
| img(image).w(width).h(height) |
| .overflow_x_scroll() | ||
| .when(needs_horizontal_scroll, |this| this.overflow_x_scroll()) | ||
| .when(!needs_horizontal_scroll, |this| this.overflow_x_hidden()) | ||
| .items_start() |
There was a problem hiding this comment.
At some point we may just want to revisit this to work better across various output types.
| /// Set to 0 to disable output width limits. | ||
| /// | ||
| /// Default: 0 | ||
| pub output_max_width_columns: usize, |
There was a problem hiding this comment.
Did you mean to take out the setting you introduced in a previous PR?
There was a problem hiding this comment.
The intention was to scale the image in the first one too -- we could leave it in but I don't see a good reason to not scale the image and have the overflow, open to suggestions tho.
|
Where are we on this @rgbkrk? |
|
We can either make this fix addititive or revert the previous merge. Would be nice to see my images fit as poorman's notebook. |
|
I can't imagine anyone is using the settings you just introduced so, let's go for it. |
|
@rgbkrk Kind reminder to merge this |
I can't merge anymore. Heck, my own PRs are sitting there waiting for review. |
|
@rgbkrk what's changed? |
|
I'm not staff and they're working on SOC2 Compliance. I just review PRs and when they've got a chance to look at community contributions within REPL they rely on me. The REPL work is not really on their roadmap. Notebooks is pushed out for post-1.0 and I think they think of the REPL in the same way. |
MrSubidubi
left a comment
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks! Apologies for the delay in reviews, need to get back up to speed
|
@MrSubidubi The failing test is not caused by this PR; seem to be related to a wiring issue (perhaps on upstream?) |
Continues zed-industries#47114 Release Notes: - Fixed REPL output width clamping to apply to the content area so images don’t get clipped by controls --------- Co-authored-by: MrSubidubi <[email protected]>
Continues zed-industries#47114 Release Notes: - Fixed REPL output width clamping to apply to the content area so images don’t get clipped by controls --------- Co-authored-by: MrSubidubi <[email protected]>
3 participants
Continues #47114
Release Notes: