Fix cut-off in text visualisations #6421

Procrat · 2023-04-25T13:49:36Z

Pull Request Description

Closes #6196.

Three things were going wrong:

Not directly contributing, but adding confusion was the fact that padding of 5px was added in two different places. Since the 10px we've had up until now looked better, especially given the size of the rounded corners, I've kept it at 10px, but only applied in one place.
The main issue was that the length the scrollbars scroll over didn't take padding into account. At the same time, I changed the max and thumb_size variables to the coordinate system of the content. This is also how they're being used in ScrollArea, which is the only other place where Scrollbars are being used.
The line height of text grid entries was set to the default of 1.2. That's the default line height in browsers, which is great for multi-line text and elements whose height is greater than the line height. In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space.

json-viz2.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the Scala, Java, and Rust style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- ~~Unit tests have been written where possible.~~
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Padding was applied in two separate places. Since it seems to look better the way it was before, I've doubled the padding value.

The core of the issue was that the length the scrollbars scroll over didn't take padding into account. At the same time, I changed the `max` and `thumb_size` variables to the coordinate system of the content. This is also how they're being used in `ScrollArea`, which is the only other place where `Scrollbar`s are being used.

The default line height in browsers is 1.2, which is great for multi-line text and elements whose height is greater than the line height. In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space.

wdanilo · 2023-04-27T03:21:16Z

...gui/view/graph-editor/src/builtin/visualization/native/text_visualization/grid_view_entry.rs

@@ -77,6 +77,7 @@ impl Entry {
        let mut style = "position: absolute; white-space: pre; pointer-events: auto;".to_string();
        write!(style, "left: {left}px; top: {top}px;").ok();
        write!(style, "width: {width}px; height: {height}px;").ok();
+        write!(style, "line-height: 1").ok();


What is this? Looks magical. If we need it, it should have doc comment with explanation.

I explained in the PR description, but I'll copy-paste it into a comment.

Hmm, if you are referring to this explanation:

The line height of text grid entries was set to the default of 1.2. That's the default line height in browsers, which is great for multi-line text and elements whose height is greater than the line height. In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space.

Then I do not understand it. I was reading it several times and it's not clear to me. default of 1.2 is great for multi-line text readability, like in our case. We should not change it to 1.0.

Anyway, you've written "In this case, however, where the height and the font size are set to the same value, the default setting of 1.2 pushes the text below the allotted space":

Height of what is the same as the font size?

What is "allotted space"?

Why these things are "pushed below" this space?

Can we make it working with default line-height?

Height of what is the same as the font size?

The height of the element this part of the code is defining: the grid view entry.

What is "allotted space"?

The vertical space that is available for the text in this element which is fixed by the height which is explicitly set in the line above.

Why these things are "pushed below" this space?

Because with a font size of 12px and a line height of 1.2, there's not a enough room in an element that's only 12px high. It needs something like 1.2 * 12px ~= 14px.

Can we make it working with default line-height?

Another option might be to make the entry size higher: not set it to any value and let the browser derive it from the font size and line height. Looking at the code, however, it looks like we always let the entry size be set externally, by the component that uses the text grid. That might be substantial change because the grid view is used in a bunch of places. Any other ideas?

Wait, can't we just change this line:
https://github.com/enso-org/enso/blob/develop/lib/rust/ensogl/app/theme/hardcoded/src/lib.rs#L590 ?

It sets the height of the grid entry as far as I can see.

@wdanilo: To make sure there's no confusion here, this PR doesn't change the distance between lines of text. The grid view entries are still fixed to the same height. This just changes where the text appears in relation to the grid view entry div.

farmaazon

Add explanation, as @wdanilo asked.

* develop: Passing events to sub-widgets in List Editor and refactoring of the slider component. (#6433) Revert "Cloud/desktop mode switcher (#6308)" (#6444) Widgets integrated with graph nodes (#6347) Table Visualization and display text changes. (#6382) Skip redundant compilations (#6436) Add parse extensions to Text type. #6330 (#6404) Cloud/desktop mode switcher (#6308) Replace Table should_equal with should_equal_verbose (#6405) Rollback event handling changes for the mouse (#6396) Dashboard directory interactivity (#6279) Ability to change the execution environment between design and live. (#6341) Implementation of elements adding to List Editor and a lot of internal API (#6390) Drop method exported from WASM + removing leaks. (#6365) Turn null into UnexpectedExpression when Union type is incomplete (#6415)

vitvakatu · 2023-04-27T18:10:34Z

QA passed 🟢

wdanilo

The text with line-height of 1.0 looks too dense. We should keep the browser's default to make it easy to read. We need to resolve the discussion before merging this PR.

farmaazon · 2023-05-02T13:31:09Z

@wdanilo this PR does not worsen the state we have already on develop, so I think we should merge it and make an issue for line spacing in text visualization. The new issue can be added as a fixme in the line you commented. Please unblock the PR.

* develop: (34 commits) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496) Finishing Vector Editor (#6470) Proper handling of multiple list views. (#6461) Fix disappearing cached shapes (#6458) Add Execution Context control to Text.write (#6459) Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443) Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442) Hide profile button behind a feature flag (#6430) ...

* develop: Fix cut-off in text visualisations (#6421) Infer correct synthetic name for nested modules (#6525) Delete unused websocket dependency (#6535) Run typecheck and eslint on `./run lint` (#6314) Force pending saves if client closes abruptly (#6514) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496)

…-5075 * develop: Build nightly 3 hours earlier. (#6551) Cache result of slow function resolution on Any that is present on a hot path (#6536) Fix cut-off in text visualisations (#6421) Infer correct synthetic name for nested modules (#6525) Delete unused websocket dependency (#6535) Run typecheck and eslint on `./run lint` (#6314) Force pending saves if client closes abruptly (#6514)

Procrat added 4 commits April 25, 2023 15:12

Remove double padding

7bd447c

Padding was applied in two separate places. Since it seems to look better the way it was before, I've doubled the padding value.

Changelog

e087e9c

Procrat marked this pull request as ready for review April 25, 2023 13:53

Procrat requested review from MichaelMauderer, wdanilo, farmaazon and kazcw as code owners April 25, 2023 13:53

wdanilo reviewed Apr 27, 2023

View reviewed changes

farmaazon approved these changes Apr 27, 2023

View reviewed changes

Procrat added 2 commits April 27, 2023 10:41

Add comment

0b6fd09

wdanilo requested changes Apr 28, 2023

View reviewed changes

wdanilo approved these changes May 3, 2023

View reviewed changes

Procrat added the CI: Ready to merge This PR is eligible for automatic merge label May 3, 2023

mergify bot merged commit 7f9ed71 into develop May 4, 2023

mergify bot deleted the wip/procrat/json-viz-last-row-6196 branch May 4, 2023 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cut-off in text visualisations #6421

Fix cut-off in text visualisations #6421

Procrat commented Apr 25, 2023 •

edited

Loading

wdanilo Apr 27, 2023

Procrat Apr 27, 2023

wdanilo Apr 28, 2023 •

edited

Loading

Procrat Apr 28, 2023

vitvakatu Apr 28, 2023

Procrat Apr 28, 2023

farmaazon left a comment

vitvakatu commented Apr 27, 2023

wdanilo left a comment

farmaazon commented May 2, 2023

Fix cut-off in text visualisations #6421

Fix cut-off in text visualisations #6421

Conversation

Procrat commented Apr 25, 2023 • edited Loading

Pull Request Description

Checklist

wdanilo Apr 27, 2023

Choose a reason for hiding this comment

Procrat Apr 27, 2023

Choose a reason for hiding this comment

wdanilo Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Procrat Apr 28, 2023

Choose a reason for hiding this comment

vitvakatu Apr 28, 2023

Choose a reason for hiding this comment

Procrat Apr 28, 2023

Choose a reason for hiding this comment

farmaazon left a comment

Choose a reason for hiding this comment

vitvakatu commented Apr 27, 2023

wdanilo left a comment

Choose a reason for hiding this comment

farmaazon commented May 2, 2023

Procrat commented Apr 25, 2023 •

edited

Loading

wdanilo Apr 28, 2023 •

edited

Loading