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

Extension-assigned Data Attributes for Explorer Tree Items #165753

Closed
shellscape opened this issue Nov 7, 2022 · 12 comments
Closed

Extension-assigned Data Attributes for Explorer Tree Items #165753

shellscape opened this issue Nov 7, 2022 · 12 comments
Assignees
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code

Comments

@shellscape
Copy link

I tried posting this in the extensions discussions but it's crickets over there

Hey all. I'm on a (what some would call futile and silly) quest to completely rework the VS Code UX to match that of Atom. It's my personal view that Atom's UX is more polished, so I'm going the effective skinning route to make that happen.

During my work on this, I've noticed that the various DecorationProvider classes in use by extensions like Git and TypeScript are able to add decoration classes which define certain properties like color. However, this is problematic for anyone inspecting the class names. It wouldn't be so bad if not for a few things, which ultimately I think were poor design choices:

  1. The tree structure is flat, rather than nested
  2. Each "level" of the tree receives a different decoration class name due to the final segment being a complex hash of a key. e.g. .monaco-decoration-iconBadge--9ieymw.
  3. Nothing but title is modified to indicate a "state" of the file wrt the git repo. e.g title="~/code/thing.ts • Untracked"

Note that 2 and 3 are specific to the git extension, but the TypeScript and other language extensions add classes for error states, etc

It seems to me that there could be a number of uses for a data attribute on the element such like data-ext-git-state="untracked". This would allow for a wider breadth of styling, like changing the tree icon based on git state, or throwing a squiggly line under files which contain errors. Right now we're relegated to CSS like this:

.monaco-icon-label[title~='Modified']::before {
  filter: brightness(0) saturate(100%) invert(84%) sepia(14%) saturate(912%) hue-rotate(351deg) brightness(94%) contrast(88%) !important;
}

That css changes the icon color to match the foreground color for modified files. It's not ideal, and it's admittedly brittle as the title attribute may change in any subsequent release of the extension. Since I don't think it would be reasonable to suggest that the decoration class code needs to be changed, I do think there's benefit in either allowing extensions to add class names, or data attribute names (preferably).

Is there anything in place that would allow for adding data attributes to indicate "state" of a tree item? If not, what does the path for adding this capability look like? I've never contributed to VS Code before, and while I'm a seasoned engineer, please do reply as if I know little about the structure of VS Code internals and extension authoring. Cheers

@alexr00
Copy link
Member

alexr00 commented Nov 10, 2022

@shellscape API that we have for extension that provide tree to indicate additional information about the tree item is the tree item's contextValue

vscode/src/vscode-dts/vscode.d.ts

Lines 10755 to 10773 in f7cfd4b

/**
* Context value of the tree item. This can be used to contribute item specific actions in the tree.
* For example, a tree item is given a context value as `folder`. When contributing actions to `view/item/context`
* using `menus` extension point, you can specify context value for key `viewItem` in `when` expression like `viewItem == folder`.
* ```json
* "contributes": {
* "menus": {
* "view/item/context": [
* {
* "command": "extension.deleteFolder",
* "when": "viewItem == folder"
* }
* ]
* }
* }
* ```
* This will show action `extension.deleteFolder` only for items with `contextValue` is `folder`.
*/
contextValue?: string;

The contextValue is used to set a context that can be used in when clauses in an extension's package.json: https://code.visualstudio.com/api/references/when-clause-contexts

Context values like this are an established pattern used throughout VS Code for enabling menu items, actions, keyboard shortcuts, etc..

Generally speaking, we strive for a cohesive look between trees, even when they're provided by different extensions, so I don't think we'd want to allow extensions start adding CSS to their tree item. The context value is the closest thing to what you're looking for, but it isn't CSS.

Relatedly, we do have a few open issues to provide more styling freedom in trees, but we don't plan to work on them right now, and given that we want to keep extension trees cohesive, they may never be something we work on. #115365, #97190

@shellscape
Copy link
Author

shellscape commented Nov 10, 2022

Thank you for that very detailed reply. I'm not sure I conveyed what I was asking clearly, or perhaps there was too much focus on the styling aspect in my issue body.

To clarify - I'm not asking how to add additional styling to VS Code. I'm asking how I can contribute code to the project to allow extensions, like the git and typescript extensions, to add data-* attributes to explorer tree html elements. I'm willing and capable of doing that work, but I'm looking for direction. I want to add these so that users can target these nodes by various means (whether it be an extension that injects CSS or an extension that would want to add additional visual context to a tree node).

The end goal would be allow users to target these nodes more accurately (by whatever means they choose), without having to rely on hashed monaco-decoration-* class names. Tangentially, I am using the Customize UI extension, but that's unrelated to the meat and potatoes of my issue here. It was probably a mistake to be so verbose about my end goals, as I know that modifying the look and feel of VS Code is a hot topic that is often brought up.

If I've misunderstood any part of your reply, please do correct me.

@daviddossett daviddossett removed their assignment Nov 10, 2022
@daviddossett
Copy link
Contributor

While I'm not the right person to talk to re: the technical asks here, I am curious about what you like about Atom's UX. Could you open a separate issue to enumerate what you like about Atom/what about VS Code isn't working for you?

@shellscape
Copy link
Author

shellscape commented Nov 10, 2022

@daviddossett there are bazillion of those issues, many of them closed and stale. I found a few last night going back to 2017 related to the find widget. If you're up for it, I think a recorded video call would be a better way to go over the differences. That would have a big advantage over a static issue. If you or anyone else would like to get one going, I can be reached at andrew at shellscape.org

@shellscape
Copy link
Author

shellscape commented Nov 10, 2022

@alexr00 the first place I noticed that might make sense as a starting point was the FileDecoration class:

constructor(badge?: string, tooltip?: string, color?: ThemeColor) {
this.badge = badge;
this.tooltip = tooltip;
this.color = color;
Perhaps adding support for data attributes as Record<string, string>

But I've run into a bit of a wall on how and where that class is being used to affect elements in the tree. Still digging, but of course any help there would be much appreciated.

Edit

It looks like maybe this is the right spot to then "render" the data attributes?

setLabel(label: string | string[], description?: string, options?: IIconLabelValueOptions): void {

@jrieken
Copy link
Member

jrieken commented Nov 14, 2022

The end goal would be allow users to target these nodes more accurately (by whatever means they choose), without having to rely on hashed monaco-decoration-* class names.

@shellscape How do target any nodes or class names? They aren't part of our API and no stability is guaranteed for any them. You have mention a "customize UI extension" - can you provide some more details here?

@shellscape
Copy link
Author

shellscape commented Nov 14, 2022

@jrieken Customize UI is an extension that's been around for just over 3 years. Its companion extension is Monkey Patch and collectively they have over 300,000 installs on the marketplace, which doesn't include installs from Open VSX.

https://marketplace.visualstudio.com/items?itemName=iocave.customize-ui
https://marketplace.visualstudio.com/items?itemName=iocave.monkey-patch

They aren't part of our API and no stability is guaranteed

This is fine. When modifying something beyond the current SLA/API Contract capability one assumes as much.

Your reply suggests that this issue is at risk of going down the age-old "but why are you doing this" programming discussion foot gun. If it's not considered wise, that's OK. If it violates some dogma or core tenant that a maintainer holds, that's OK too.

I'm a user, I have a clear need, I want to contribute and the assistance and direction I'm asking for is purely technical. This wasn't an issue opened for a feature request, bug report, or merit discussion. I'm asking for assistance in contributing code - even if that is ultimately rejected as part of the review process. My prior reply #165753 (comment) had some questions on the right place in code to be looking and it would be very cool if that could get some eyeballs on it. As a first-time contributor and forced-migrant from Atom, the friction in getting a technical reply is a bit frustrating.

@daviddossett
Copy link
Contributor

@daviddossett there are bazillion of those issues, many of them closed and stale. I found a few last night going back to 2017 related to the find widget. If you're up for it, I think a recorded video call would be a better way to go over the differences. That would have a big advantage over a static issue.

I'd suggest posting a recording as part of a new issue if it's easier than writing it all out. That's a much easier way to get more eyes and feedback on the topic than a siloed meeting recording.

@shellscape
Copy link
Author

But I wasn't asking for a siloed meeting. Why not take the recording of the meeting and put it on the roadmap? This feels very much toss-the-ball-over-the-fence. With my time on the Rollup project, I've taken several video calls with users who face issues that there just aren't good reproductions for, and that's been an immense help. I'm not even paid by a company for that time.

there are bazillion of those issues, many of them closed and stale

What factor should give me comfort that if I take the time to record a video highlighting these things, that it won't end up in the heap with all of the other stale issues? What I'm asking for is that a member of the vscode team who is interested in this kind of feedback meet with me, to invest their time just as you're asking me to - time they'll be paid for as they're employed by Microsoft to work on it. There's no loss there. I don't believe it's a big ask for 30 minutes or so that someone is being paid for, especially if you all truly value deep user input like this. Additionally if there's a vscode team member putting their time to this, it's going to have a much higher chance of actually being considered.

I'm here, and I'll make time for you all whenever is good for whoever. Let's make that reciprocal.

@isidorn isidorn assigned isidorn and unassigned lramos15 and alexr00 Dec 7, 2022
@isidorn isidorn added the feature-request Request for new features or functionality label Dec 7, 2022
@isidorn
Copy link
Contributor

isidorn commented Dec 7, 2022

Unassigning Logan and Alex and assigning to me since this is also a product decision.

We always appreciate interest in making a contribution, but there are some cases where, even if the feature was implemented flawlessly and required no maintenance from us, we might not want to accept a PR for it. This is one of those cases.

Our API is, broadly speaking (and maybe with a few exceptions), concerned with data and features and not with allowing you to modify the UI. To keep VS Code looking like VS Code, we don't want to add the kind of UI API that you're suggesting. We would not accept a PR for this change.

Obviously this isn't the response you wanted to hear, but we do still like to hear from users who want more features, even when we disagree on whether we should have those features. This feedback can point us in a related direction that can make VS Code better even if we don't add the original feature request. It also leaves a record for other about why we made a specific decision. So, thank you for being a VS Code user and engaging with us.

I am going to go ahead and close this issue as out of scope.

@vscodenpa vscodenpa added this to the Backlog Candidates milestone Dec 7, 2022
@isidorn isidorn added the *out-of-scope Posted issue is not in scope of VS Code label Dec 7, 2022
@vscodenpa
Copy link

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!

@vscodenpa vscodenpa closed this as not planned Won't fix, can't repro, duplicate, stale Dec 7, 2022
@microsoft microsoft deleted a comment from vscodenpa Dec 7, 2022
@shellscape
Copy link
Author

shellscape commented Dec 7, 2022

To keep VS Code looking like VS Code

Why is Microsoft so dogmatic about this?

What does Microsoft gain by asserting control over the UX and appearance of VS Code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

No branches or pull requests

8 participants