-
Notifications
You must be signed in to change notification settings - Fork 138
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
dock ui fixes #187
dock ui fixes #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 4a06629 in 2 minutes and 21 seconds
More details
- Looked at
74
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
12
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:65
- Draft comment:
Changed padding from '4px 8px' to '4px'. Ensure reducing horizontal padding doesn't negatively affect icon alignment. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that a change in padding doesn't negatively affect icon alignment. This is a request for confirmation of intention, which violates the rule against asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:106
- Draft comment:
Width for the creator icon container is now 67px (was 75px). Verify that this aligns with the new label length ('Agent') and overall UI spacing. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify alignment with the new label length and UI spacing, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:136
- Draft comment:
Added z-index on hover for inactive icons. Confirm that 40000 doesn't conflict with other layered elements in the UI. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a speculative comment asking for verification rather than pointing out a concrete issue. It asks the author to "confirm" something, which violates our rules. While z-index conflicts can be real issues, without specific evidence of a conflict, this is just speculation. The author likely chose 40000 deliberately and tested it in their UI.
Maybe there are z-index conflicts in other files that we can't see. The z-index value seems unusually high which could be a red flag.
Even if there are z-index conflicts, we should trust that the author tested their UI changes. The rules state we should ignore cross-file issues and not make speculative comments.
Delete this comment. It's speculative, asks for verification, and doesn't point out a concrete issue. It violates multiple comment rules.
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:158
- Draft comment:
Removed the explicit color styling for the label element; ensure that text color is handled appropriately via parent styles or theme variables. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:225
- Draft comment:
Explicitly setting icon colors with !important may override theme-based styling. Confirm that the hardcoded color values (#E9FFC7, #FFCEE3, etc.) are acceptable. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to confirm if the hardcoded color values are acceptable, which is not allowed according to the rules. However, it does point out a potential issue with using !important in CSS, which could be useful information. The comment could be rephrased to suggest a change or improvement without asking for confirmation.
6. src/vs/workbench/browser/parts/paneCompositePart.ts:121
- Draft comment:
Updated the inventory icon label from 'Creator' to 'Agent'. Ensure that corresponding UI views and commands reflect this label change. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that corresponding UI views and commands reflect the label change. This is a request for confirmation and testing, which violates the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
7. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:67
- Draft comment:
Changing padding from '4px 8px' to '4px' removes horizontal spacing. Verify that this aligns with the intended design. - Reason this comment was not posted:
Marked as duplicate.
8. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:109
- Draft comment:
Width for the inventory-creator icon container changed from 75px to 67px. Confirm that the new width properly accommodates the icon and label per design specs. - Reason this comment was not posted:
Marked as duplicate.
9. src/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:137
- Draft comment:
Added a high z-index (40000) on hover for inactive icons. Please verify that this does not interfere with stacking or cause unexpected overlaps. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. src/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:159
- Draft comment:
Removal of the static text color on '.icon-label' allows child-specific colors to take effect. Ensure the inherited colors maintain good contrast and readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. src/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:225
- Draft comment:
Explicitly setting colors for each inventory icon container (with !important) improves consistency, but consider whether !important is necessary and that color contrast requirements are met. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. src/vs/workbench/browser/parts/paneCompositePart.ts:121
- Draft comment:
Changed the label from 'Creator' to 'Agent' for the inventory icon. Ensure this update is reflected throughout the UI and documentation. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rbKJRNdGsgGXxPaF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 71b4dd6 in 54 seconds
More details
- Looked at
22
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:70
- Draft comment:
Is reducing the gap from 8px to 4px intentional? Verify if the tighter spacing works well across all layouts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment asks the PR author to verify if the tighter spacing works well across all layouts, which violates the rule against asking the author to ensure behavior is intended or tested. However, it does ask if the reduction in gap is intentional, which is a valid point.
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:150
- Draft comment:
Increasing the icon background-size from 16px to 18px may affect icon alignment. Confirm design intent. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to confirm the design intent, which violates the rule against asking for confirmation of intention. However, it does point out a potential issue with icon alignment, which could be useful. The comment could be rephrased to suggest checking the alignment without asking for confirmation.
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:70
- Draft comment:
Confirm the reduction of gap from 8px to 4px aligns with the intended layout spacing. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:150
- Draft comment:
Verify that changing background-size from 16px to 18px produces the desired icon scaling across all scenarios. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_VIPRA7cwb5DqL8bw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 08ef64d in 30 seconds
More details
- Looked at
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:253
- Draft comment:
Consider using an integer pixel value (e.g., 1px) instead of 0.5px for margin-left, as fractional pixels may render inconsistently across browsers. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:258
- Draft comment:
Consider using an integer pixel value (e.g., 1px) for consistency and to avoid potential subpixel rendering issues. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:253
- Draft comment:
Using a fractional margin (0.5px) can lead to inconsistent rendering in some browsers. Consider adding a comment explaining the need for this subtle shift. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:258
- Draft comment:
Consider documenting why a 0.5px margin is necessary, as fractional pixels may render differently across devices. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_dN83Ia3tb6qZRwJS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 12c24c7 in 58 seconds
More details
- Looked at
43
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:82
- Draft comment:
Avoid hardcoding z-index without explanation. Provide a comment on why z-index: 3 is needed here. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:103
- Draft comment:
Using !important for z-index: 2 may indicate potential stacking conflicts. Confirm if this override is necessary. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:141
- Draft comment:
Removal of high z-index in hover state: verify that the hover stacking order remains correct. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:85
- Draft comment:
Base .icon-container now has an explicit z-index (3). Verify that this stacking order relative to active states (which use a lower z-index) is intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:105
- Draft comment:
Active icon containers now receive 'z-index: 2 !important'. Ensure that lowering their z-index (compared to the base of 3) and using !important is the desired behavior for all active icons. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:142
- Draft comment:
The high z-index (40000) has been removed from the hover effect on inactive icons. Confirm that this removal does not adversely affect the intended layering during hover. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_Ytt2vUhzjNRBhW2s
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 79bb4b0 in 2 minutes and 18 seconds
More details
- Looked at
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:127
- Draft comment:
Double-check the visual impact and performance of multiple box-shadow definitions. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:127
- Draft comment:
Consider using a CSS variable or common mixin for the repeated inner border effect (#FFFFFF80) to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_9Kk5VoDkAHeQzbRt
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
width: 80px !important; | ||
} | ||
|
||
/* Glow effects only on active state */ | ||
.monaco-workbench .part.auxiliarybar .icon-group .icon-container.active:has(.inventory-chat-icon) { | ||
box-shadow: 0px 0px 57.6px 9.6px #AFF349; | ||
box-shadow: 0px 0px 57.6px 9.6px #AFF349, /* Outer glow */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the repeated inner border effect (inset shadow) into a CSS variable for maintainability.
Important
UI adjustments in CSS for better consistency and a label change in TypeScript from 'Creator' to 'Agent'.
auxiliaryBarPart.css
: Reduced padding from4px 8px
to4px
and gap from8px
to4px
in.icon-group
.z-index: 3
to.icon-container
andz-index: 2
to active icon containers.background-size
from16px
to18px
for.icon-element
.paneCompositePart.ts
: Renamedlabel
fromCreator
toAgent
inInventoryIcons
.This description was created by
for 79bb4b0. It will automatically update as commits are pushed.