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

Make telemetry info table a little bit narrower and aligned #233961

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mattmaniak
Copy link

@mattmaniak mattmaniak commented Nov 15, 2024

The current table in defaultSettings.json was formatted using variables from the telemetryService.ts which are longer than the variables' content itself.

Fixes: #233959

Before:

	// |       | Crash Reports | Error Telemetry | Usage Data |
	// |:------|:---------------------:|:---------------:|:--------------:|
	// | all   |            ✓          |        ✓        |        ✓       |
	// | error |            ✓          |        ✓        |        -       |
	// | crash |            ✓          |        -        |        -       |
	// | off   |            -          |        -        |        -       |

After:

	// |       | Crash Reports | Error Telemetry | Usage Data |
	// |:------|:-------------:|:---------------:|:----------:|
	// | all   |       ✓       |        ✓        |     ✓      |
	// | error |       ✓       |        ✓        |     -      |
	// | crash |       ✓       |        -        |     -      |
	// | off   |       -       |        -        |     -      |

@mattmaniak mattmaniak changed the title Make telemetry info table a little bit narrower but aligned Make telemetry info table a little bit narrower and aligned Nov 15, 2024
@lramos15
Copy link
Member

Can you attach a photo of what the table now looks like in the settings UI? As it appears aligned already there and that was the case we were optimizing for as that is where most people edit their settings.

@lramos15 lramos15 assigned lramos15 and unassigned joyceerhl Nov 18, 2024
@mattmaniak
Copy link
Author

Here it is how it looks by default:
before

...and how it looks like this after my fix:
after

The alignment is present but only for the defaultSettings.json generator. Look that the variables that hold column titles are longer that the titles itself, eg. ${crashReportsHeader} is longer than the Crash Reports title:
source

@lramos15
Copy link
Member

lramos15 commented Nov 18, 2024

I mean in the settings UI. I just want to ensure your fix doesn't regress this UI. I don't believe it will as markdown tables don't care about spacing, but it is good to validate

image

@mattmaniak
Copy link
Author

In Settings UI it looks perfectly as above. It was kinda misleading because I keep to manage my configuration from .json mostly rather than UI. But if it would be possible to keep Settings UI aligned and defaultSettings.json also it would be great. However I suppose that defaultSettings.json is somehow used for rendering UI. Am I right?

@lramos15
Copy link
Member

However I suppose that defaultSettings.json is somehow used for rendering UI. Am I right?

That's right, they're the same

@lramos15 lramos15 added this to the November 2024 milestone Nov 18, 2024
@mattmaniak
Copy link
Author

Perfectly as above - without my changes currently. I will switch enviro and write down the status cause I made a quick check on my second station. I thought that you mean if there is a UI bug by default on my instance. Sorry for the potential confusion.

@mattmaniak
Copy link
Author

I can confirm that the change does not regress the Settings UI table so we can proceed.
build

@lramos15
Copy link
Member

@mattmaniak Is there anyways we can make the | aligned as well? I see in the default.json they're now

@mattmaniak
Copy link
Author

I will report such when found in a separate Issue/PR because I am not sure where to locate every generated table in the editor.

@lramos15
Copy link
Member

I just meant for the telemetry table, not all of them. If we're fixing the alignment in this PR. We might as well fix the alignment completely and ensure the column separators are properly aligned.

@mattmaniak
Copy link
Author

mattmaniak commented Nov 20, 2024

Understood. I don't see a solution for aligning the "text" column fully in json as the checkmark sign (✓) seems to be a little wider than the rest of monospace characters. Column separators have the same number of spaces before and after dash and checkmark signs. As an exception, GitHub Mobile treats the checkmark as a fully monospaced character...
IMG_0869

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.

Telemetry info table is not aligned in defaultSettings.json
3 participants