Wip/mk/whitespace chars monospace font table viz #10563

marthasharkey · 2024-07-16T09:41:26Z

Pull Request Description

closes Add white space rendering to Text fields in Table viz #10247

Partial (default) setting:

Full:

Off:

Dropdown:

Important Notes

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,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

AdRiley · 2024-07-16T14:25:46Z

What are the 3 modes? I think I was expecting 2 modes with a single on/off toggle button.

marthasharkey · 2024-07-16T14:48:55Z

What are the 3 modes? I think I was expecting 2 modes with a single on/off toggle button.

I thought there was going to be on/off and then the 'special' partial one, maybe I misunderstood and we want just on and the 'partial' setting?

jdunkerley · 2024-07-16T18:27:07Z

Looks really good and close to the target.

We did talk about the 3 modes.
I think the off mode should still be fixed width and spaces and tabs taking the correct space but not replaced with grey text.
I imagined a dropdown rather than radio buttons (we can then have a little descriptive text).

radeusgd · 2024-07-18T13:08:36Z

The Full mode screenshot is not loading for me.

radeusgd · 2024-07-18T13:09:34Z

I'm so sorry for the accidental close - I clicked something accidentally 🤦 - reopened

kazcw · 2024-07-18T12:42:23Z

app/gui2/src/components/VisualizationContainer.vue

@@ -285,7 +285,7 @@ const contentStyle = computed(() => {
  max-width: calc(100% - var(--permanent-toolbar-width));
  /* FIXME [sb]: This will cut off floating panels - consider investigating whether there's a better
   * way to clip only the toolbar div itself. */
-  overflow-x: hidden;
+  overflow: visible;


After this change, does the toolbar still behave correctly when the visualization is very narrow? If so, I guess the comment above can be deleted.

kazcw · 2024-07-18T12:51:08Z

app/gui2/src/components/visualizations/TableVisualization.vue

+function getRowHeight(params: RowHeightParams) {
+  if (textFormatterSelected.value === TextFormatOptions.Off) return DEFAULT_ROW_HEIGHT
+  const rowData = Object.values(params.data)
+  const textValues = rowData.filter((r) => typeof r === 'string') as string[]


Suggested change

const textValues = rowData.filter((r) => typeof r === 'string') as string[]

const textValues = rowData.filter<string>((r) => typeof r === 'string')

This was marked as resolved but not addressed.

app/gui2/src/components/visualizations/TableVisualization.vue

app/gui2/src/components/TextFormattingSelector.vue

radeusgd · 2024-07-18T13:11:47Z

Looking at the Partial screenshot - I feel like it is not that easy to distinguish the whitespace marks from regular characters. What if I had a text containing the ··· centered dot characters? Could we maybe use some more indicative styling, maybe making it light-gray? What do you all think?

marthasharkey · 2024-07-18T14:37:44Z

Looking at the Partial screenshot - I feel like it is not that easy to distinguish the whitespace marks from regular characters. What if I had a text containing the ··· centered dot characters? Could we maybe use some more indicative styling, maybe making it light-gray? What do you all think?

here is and example using 'lightgrey'

app/gui2/src/components/TextFormattingSelector.vue

Co-authored-by: Kaz Wesley <[email protected]>

kazcw

Please avoid force-pushing any non-draft branch; GitHub isn't able to show the changes since a previous review when there has been a force-push, so we use merge rather than rebase to keep branches up to date.

kazcw · 2024-07-22T13:25:11Z

app/gui2/src/components/TextFormattingSelector.vue

+        :title="`Text displayed in monospace font, only multiple spaces displayed with &#183;`"
+        @click.stop="() => emit('changeFormat', TextFormatOptions.Partial)"
+      />
+      <button


All of our buttons have tooltips and some consistent styling. I see this one matches the styles by copy-and-paste, but that won't stay up to date if we change the rest of the buttons. This button too could be rendered with one of our button abstractions:

It could be a SvgButton. The icon can be styled using a deep-style selector: .OffButton :deep(.SvgIcon) { /* strikethrough styles */ }

Or it could be done with our most general button, MenuButton, which can be applied to anything to provide the button styling and tooltip.

I have used Menu Button, but for all entries as this seemed to make it easier to have consistent formatting

kazcw · 2024-07-22T13:26:59Z

app/gui2/src/components/VisualizationContainer.vue

-  /* FIXME [sb]: This will cut off floating panels - consider investigating whether there's a better
-   * way to clip only the toolbar div itself. */


It's always nice to see a FIXME fixed 😄

kazcw · 2024-07-22T13:29:13Z

app/gui2/src/components/visualizations/TableVisualization.vue

+function getRowHeight(params: RowHeightParams) {
+  if (textFormatterSelected.value === TextFormatOptions.Off) return DEFAULT_ROW_HEIGHT
+  const rowData = Object.values(params.data)
+  const textValues = rowData.filter((r) => typeof r === 'string') as string[]


This was marked as resolved but not addressed.

…ble-viz

marthasharkey force-pushed the wip/mk/whitespace-chars-monospace-font-table-viz branch 2 times, most recently from dfcb990 to 2806102 Compare July 18, 2024 10:06

marthasharkey marked this pull request as ready for review July 18, 2024 10:48

marthasharkey requested review from Frizi, farmaazon, vitvakatu, kazcw and AdRiley as code owners July 18, 2024 10:48

radeusgd closed this Jul 18, 2024

radeusgd reopened this Jul 18, 2024

kazcw reviewed Jul 18, 2024

View reviewed changes

marthasharkey force-pushed the wip/mk/whitespace-chars-monospace-font-table-viz branch from 4e91cf6 to d79b08f Compare July 18, 2024 13:41

somebody1234 reviewed Jul 18, 2024

View reviewed changes

app/gui2/src/components/TextFormattingSelector.vue Outdated Show resolved Hide resolved

marthasharkey requested a review from kazcw July 21, 2024 15:04

marthasharkey added 9 commits July 22, 2024 09:33

add monospace font on text cells

167364b

add function to format text

93f1dd2

getRowHeight calculated

13224b6

sort double returns

0801c9f

toolbar buttons

60b5eb5

simplify format text function

d87b1bb

only multiple spaces rendered as dots in special mode

99cca0a

simplify format text function, reduce duplicated code

33a1308

add function for other whitespace

36c6c73

marthasharkey and others added 17 commits July 22, 2024 09:33

render whitespace

46af593

start of dropdown

0dc23d1

drop down showing

d07a1ca

dropdown with small text

e011be1

rename enum

f69ed68

update space rendering

e82cad2

Update app/gui2/src/components/visualizations/TableVisualization.vue

aec01e8

Co-authored-by: Kaz Wesley <[email protected]>

Update app/gui2/src/components/TextFormattingSelector.vue

91efaec

Co-authored-by: Kaz Wesley <[email protected]>

Update app/gui2/src/components/TextFormattingSelector.vue

c866c85

Co-authored-by: Kaz Wesley <[email protected]>

Update app/gui2/src/components/TextFormattingSelector.vue

3919ca2

Co-authored-by: Kaz Wesley <[email protected]>

use SVG icon and svg button

faa19c3

improves style when using svg components

2805d99

sort overflow problem

686f441

fix typo

6e37fc7

move classes to viz container

264cfc3

remove unused import

7457c59

sort type problems

54a16b6

marthasharkey force-pushed the wip/mk/whitespace-chars-monospace-font-table-viz branch from 5ac8373 to 54a16b6 Compare July 22, 2024 08:36

marthasharkey added 3 commits July 22, 2024 09:56

sort tab spacing

0f16fe2

remove changes to warnin viz

fbb0faa

use lightgrey

7e019fd

kazcw approved these changes Jul 22, 2024

View reviewed changes

marthasharkey added 4 commits July 22, 2024 14:48

add changelog

ea1f559

use MenuButton

f3144f3

sort types

4294ac1

Merge branch 'develop' into wip/mk/whitespace-chars-monospace-font-ta…

123ffbb

…ble-viz

AdRiley mentioned this pull request Jul 23, 2024

July 2024 Release Requirements #10638

Closed

jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jul 23, 2024

mergify bot merged commit 8af3fd4 into develop Jul 23, 2024
33 of 36 checks passed

mergify bot deleted the wip/mk/whitespace-chars-monospace-font-table-viz branch July 23, 2024 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wip/mk/whitespace chars monospace font table viz #10563

Wip/mk/whitespace chars monospace font table viz #10563

marthasharkey commented Jul 16, 2024 •

edited

Loading

AdRiley commented Jul 16, 2024

marthasharkey commented Jul 16, 2024 •

edited by somebody1234

Loading

jdunkerley commented Jul 16, 2024

radeusgd commented Jul 18, 2024

radeusgd commented Jul 18, 2024 •

edited

Loading

kazcw Jul 18, 2024

kazcw Jul 18, 2024

kazcw Jul 22, 2024

radeusgd commented Jul 18, 2024

marthasharkey commented Jul 18, 2024 •

edited by somebody1234

Loading

kazcw left a comment

kazcw Jul 22, 2024

marthasharkey Jul 22, 2024

kazcw Jul 22, 2024

kazcw Jul 22, 2024

	const textValues = rowData.filter((r) => typeof r === 'string') as string[]
	const textValues = rowData.filter<string>((r) => typeof r === 'string')

		/* FIXME [sb]: This will cut off floating panels - consider investigating whether there's a better
		* way to clip only the toolbar div itself. */

Wip/mk/whitespace chars monospace font table viz #10563

Wip/mk/whitespace chars monospace font table viz #10563

Conversation

marthasharkey commented Jul 16, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

AdRiley commented Jul 16, 2024

marthasharkey commented Jul 16, 2024 • edited by somebody1234 Loading

jdunkerley commented Jul 16, 2024

radeusgd commented Jul 18, 2024

radeusgd commented Jul 18, 2024 • edited Loading

kazcw Jul 18, 2024

Choose a reason for hiding this comment

kazcw Jul 18, 2024

Choose a reason for hiding this comment

kazcw Jul 22, 2024

Choose a reason for hiding this comment

radeusgd commented Jul 18, 2024

marthasharkey commented Jul 18, 2024 • edited by somebody1234 Loading

kazcw left a comment

Choose a reason for hiding this comment

kazcw Jul 22, 2024

Choose a reason for hiding this comment

marthasharkey Jul 22, 2024

Choose a reason for hiding this comment

kazcw Jul 22, 2024

Choose a reason for hiding this comment

kazcw Jul 22, 2024

Choose a reason for hiding this comment

marthasharkey commented Jul 16, 2024 •

edited

Loading

marthasharkey commented Jul 16, 2024 •

edited by somebody1234

Loading

radeusgd commented Jul 18, 2024 •

edited

Loading

marthasharkey commented Jul 18, 2024 •

edited by somebody1234

Loading