Skip to content

Remove global list styles#228173

Merged
mjbvz merged 3 commits intomicrosoft:mainfrom
mjbvz:querulous-takin
Sep 12, 2024
Merged

Remove global list styles#228173
mjbvz merged 3 commits intomicrosoft:mainfrom
mjbvz:querulous-takin

Conversation

@mjbvz
Copy link
Copy Markdown
Collaborator

@mjbvz mjbvz commented Sep 10, 2024

I'm trying to write a CSS rule that allows codicon in chat to render as normal. My previous @layer workaround caused a regression in other lists

The root problem I was trying to work around was that there's no way to clear rules that are set in the global list style sheet. You can override them with more specific css rules, but you can't remove them so that other global css rules (such as those for setting file icon colors) work properly

After discussion it sounds like we can try removing the global list styles entirely as they may not be used

Previous `@layer` fix caused a regression

The root problem I was trying to work around was that there's no way to clear rules that are set in the global list style sheet. You can override them with more specific css rules, but you can't remove them so that other global css rules (such as those for setting file icon colors) work properly

Instead of making the default styles apply to every list, it seems like they should only apply the lists the don't have their own styling. I've change it so that you now have to use `default-styled-list` if you want the default styles to apply

This seems to work but there are a ton of places where we use lists. Open to other ideas too
@mjbvz mjbvz added this to the September 2024 milestone Sep 10, 2024
@mjbvz mjbvz requested a review from aeschli September 10, 2024 23:50
@mjbvz mjbvz self-assigned this Sep 10, 2024
Copy link
Copy Markdown
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got this old issue- #197574

It looks like we always apply the default styles to the individual list's style controller-

this.style(styles ? getListStyles(styles) : defaultListStyles);
so maybe the global one is totally redundant?

this.updateStyles(options.overrideStyles);
}

this.getHTMLElement().classList.toggle('default-styled-list', !options.overrideStyles);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if I only set one color in overrideStyles, then do I lose all the default colors?

@mjbvz
Copy link
Copy Markdown
Collaborator Author

mjbvz commented Sep 11, 2024

Thanks @roblourens! Removing the global styles would be best and it seems to work fine in quick testing. I was worried about breaking some part of the UI I hadn't tested tho, which is why I went with the current approach

@aeschli @joaomoreno What do you think about trying to remove the global styles entirely?

@roblourens
Copy link
Copy Markdown
Member

I previously talked to Martin on Slack and Joao via #197574 and I don't think anyone knows why we would still need the global styles. I think we should just look at the code carefully, delete it, and audit the styles on lists including ones that aren't WorkbenchList. I can spend a little time on this today

@mjbvz mjbvz changed the title Try another fix for chat codicon styling issue Remove global list styles Sep 11, 2024
roblourens
roblourens previously approved these changes Sep 12, 2024
@mjbvz mjbvz merged commit f0fe13e into microsoft:main Sep 12, 2024
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Oct 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants