Settings: Improve titles #5791
Settings: Improve titles #5791
Conversation
|
E2E Tests 🚀 ? |
|
To make it easier to review this PR, I would recommend hiding whitespace to only see relevant changes to updating setting titles. |
| "r.configuration.title": "Positron R Language Pack", | ||
| "r.configuration.title-dev": "Positron R Language Pack (advanced)", | ||
| "r.configuration.title": "R", | ||
| "r.configuration.title-dev": "Advanced Settings", |
There was a problem hiding this comment.
I am considering changing this title to: Advanced since the Settings portion is a bit redundant. Let me know if anyone strongly disagrees. The contribution point docs mention avoiding these terms since they are redundant as well: https://code.visualstudio.com/api/references/contribution-points#contributes.configuration
There was a problem hiding this comment.
Yes, let's not use "Settings" 👍
There was a problem hiding this comment.
This looks fantastic! The state of our settings has been bothering me for soooooo long. 😭
I especially am happy about the rename to "App Launcher". Can you open up an issue to update these docs?
Also, since this is such an extensive set of changes, can you partner with QA to make sure we are running an extremely comprehensive set of tests on this PR and that we are all green before merging?
| "r.configuration.title": "Positron R Language Pack", | ||
| "r.configuration.title-dev": "Positron R Language Pack (advanced)", | ||
| "r.configuration.title": "R", | ||
| "r.configuration.title-dev": "Advanced Settings", |
There was a problem hiding this comment.
Yes, let's not use "Settings" 👍
| "positron.runApplication.showEnableShellIntegrationMessage": { | ||
| "positron.appLauncher.showEnableShellIntegrationMessage": { |
There was a problem hiding this comment.
I noticed that we have quite a number of settings that live under the positron prefix in the JSON file for settings. It's a little odd from a UX perspective for some setting IDs to have a positron prefix. From a DX standpoint, its helpful but we may want to consider removing this prefix and adding some guidelines to the wiki on naming these properties. This can be something we do in the future.
What do others think?
There was a problem hiding this comment.
I do not have strong opinions on this, except that I don't think it is nearly as important as what folks see in the UI. Maybe not urgent enough to do right now, and instead we can open an issue to reexamine later?
I do think it's possible this would be a difficult change to make because many, many beta users have these settings in their settings.json already with their original names.
There was a problem hiding this comment.
I do not have strong opinions on this, except that I don't think it is nearly as important as what folks see in the UI. Maybe not urgent enough to do right now, and instead we can open an issue to reexamine later?
I agree that it doesn't feel urgent. I'll drop a note about this in the tagged issue for this PR since it's all settings related.
I do think it's possible this would be a difficult change to make because many, many beta users have these settings in their settings.json already with their original names.
Totally agree that it is a difficult change but I would argue that its better to make this change while we are in beta. It would be more acceptable before an official release of the product. It's definitely lower priority and maybe not worth the effort to change if it will cause disruption for users without much gains. I was just curious if other folks agreed/disagreed.
Created an issue to update the docs page: #5824
I chatted with QA and we'll run the full test suite on this to make sure none of the settings were broken in the process of the rename. We don't have setting specific tests but there are a number of tests that update settings which should cover the changes. |
dhruvisompura
force-pushed
the
dhruvisompura/1487-audit-settings-titles
branch
from
a0011d2 to
53afb9a
Compare
Description
Addressess #1487
This is a first pass at improving the setting titles displayed in the settings table of contents for built in positron extensions. Any titles prefixed with "Positron" has been removed and ambiguity in extension titles have been addressed. The setting title in the TOC and category title for each setting item has been unified to match where possible. configuration keys have been changed to fix formatting of setting categories where possible.
Extension configuration keys that are prefixed with
positronhave been left as is for now (excluding theInterpreter settings) since the prefix does not effect the settings UI for now. We may want to address improving the key names for folks who interact with settings via the JSON file. It may be worth tackling after settling on a standard for configuration name formatting for bundled extensions.The "Positron > Interpreters" settings now live under "Interpreters"/
The following extensions were modified and have the following setting titles in the TOC:
QA Notes
We'll want to verify there aren't any regressions with the settings for the extensions listed above, excluding any experimental feature settings.
For the interpreter settings we should verify that changing the automatic startup behavior, restart on crash behavior, and viewing sessions for interpreters are respected as usual. Some of this behavior should be captured by our e2e tests which will be run against this change before merging.
Note that as part of this change, the qa-example-content repo did need a change due to a setting key being renamed: posit-dev/qa-example-content#41
Screenshots
Screen.Recording.2024-12-17.at.4.06.04.PM.mov