improve layout of the prompt attachment #239514
Conversation
.../workbench/contrib/chat/browser/attachments/instructionsAttachment/instructionsAttachment.ts
Show resolved
Hide resolved
| .chat-attached-context .chat-prompt-instructions-attachment:focus .monaco-button { | ||
| outline: none; | ||
| border-color: var(--vscode-focusBorder); | ||
| } |
There was a problem hiding this comment.
Otherwise the border stays the same color while all others change to blue. That is the same logic we have for existing borders extended to this one.
There was a problem hiding this comment.
I would have expected this to be handled by some general .monaco-button:focus rule? If you're copying this from elsewhere all good, but it looks like a potential code smell to me.
There was a problem hiding this comment.
Yeah I'd like it to be general component that I could reuse, but its not the case rn 🤔
Having a style that will allow this would in the monaco styles themself would be redundant and might feel like a code smell on its own - these styles are not for the monaco-buttom focus, but focus of the parent. Hence such styles would be something like *:focus .monaco-button {} which would not only have a negative performance impact, but also would require these styles to be reset for the components that don't need them 🤔
We also have many similar styles across the codebase (and many codesmells indeed!), including changes to the styles of its border (e.g., here) 🐰
| @@ -987,6 +1003,11 @@ have to be updated for changes to the rules above, or to support more deeply nes | |||
| .chat-attached-context .chat-prompt-instructions-attachments .chat-prompt-instructions-attachment.warning.implicit { | |||
| border: 1px solid currentColor; | |||
| } | |||
| .chat-attached-context .chat-prompt-instructions-attachments .chat-prompt-instructions-attachment.implicit .monaco-button { | |||
| margin-top: -1.5px; | |||
There was a problem hiding this comment.
You shouldn't need to get into decimals for margins?
There was a problem hiding this comment.
Why not? The CSS spec allow that and it makes a difference in rendering at least on the high definition screens. These are the "device" pixels, not the "screen" ones, right?
The alternative fix would involve changing the hight of the entire attachment element which is much more disruptive change. I plan to do it at some point but that change needs more time and broader discussion with the team 🤷
There was a problem hiding this comment.
These are "css pixels", not "device pixels. Meaning on low density displays this will probably shift the content 0.5px upwards and likely result in blurry horizontal b.orders
There was a problem hiding this comment.
Right what I meant is - these are CSS pixels that are mapped to "device" pixels, that finally mapped to the "screen" pixels. Furthermore, all pixel values are represented as in ones, which adds a bit of indirection and can affect the final result.
So not sure how common the 1:1 density screens are still out there, but even if we assume there is an substantial amount of them, there are so many things that are out of our control that almost certainly will result in some blurred parts of the UI 🤷
There was a problem hiding this comment.
Removed this margin-top change, I don't think we need it anymore.
| .chat-attached-context .chat-prompt-instructions-attachment .monaco-icon-label-container { | ||
| margin-top: -1px; | ||
| } |
There was a problem hiding this comment.
Can this be solved a different way other than margin top? Is the thing above it too large?
There was a problem hiding this comment.
I can try to use a line-height(or even top) property instead but that might require an increased specificity of the CSS selector to be able to override the existing styles 😞
There was a problem hiding this comment.
A negative margin top doesn't change the element's position in the flow of the page, meaning it's kind of like adding 1px padding to the bottom. We own everything here, so it might be more appropriate to change something else instead?
Why are these particular attachments special compared to other attachments such that they would be positioned 1px higher? Doesn't that mean they're going to be misaligned with regular attachments?
There was a problem hiding this comment.
Margin top does not change how the flow model for elements that use them, but it does change position of the element on the page in all layout models, see https://codepen.io/sol0mka/pen/emOaZvB. The middle example is for the case when elements are "removed" from the normal flow, as you can see, there is no padding bottom, and the element position affect other elements 🤗
Why are these particular attachments special compared to other attachments such that they would be positioned 1px higher? Doesn't that mean they're going to be misaligned with regular attachments?
It is the attachment that I own 🤷 I'd like all our attachments to reuse the same component/styles, and the last time I've tried to touch someone else's code, I was stuck in PR feedback for a month 😞 So I decided to improve the style for at least the components that I have a control of
There was a problem hiding this comment.
Btw we also have a lot of mt: -1px and top: -1px across the code base 🙈 😄 (e.g., here)
There was a problem hiding this comment.
Changed this to using ems instead since that expresses the intent better - we want to have center based on the x-hight of the text. This is the simplest option we have, the alternatives are feel hacky and verbose 🤓
legomushroom
deleted the
legomushroom/prompts/attachment-ui-improvement
branch
Improve the CSS styles of the prompt attachment: