-
Notifications
You must be signed in to change notification settings - Fork 125
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
new header bar #177
base: main
Are you sure you want to change the base?
new header bar #177
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.
❌ Changes requested. Reviewed everything up to f64f65b in 2 minutes and 16 seconds
More details
- Looked at
212
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:227
- Draft comment:
Good use of multiple header containers with explicit ordering that matches CSS ordering. Ensure the container ordering remains consistent with styling expectations. - Reason this comment was not posted:
Confidence changes required:0%
None
2. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryTitleBar.ts:31
- Draft comment:
The title bar instance creation and button rendering are clear. Confirm that usingCodicon
withicon.id
will always yield valid class names. - Reason this comment was not posted:
Confidence changes required:33%
None
3. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:230
- Draft comment:
Good addition of 'composite' and 'header' classes for styling. Consider adding a brief inline comment explaining why these classes are required, to ease future maintenance. - Reason this comment was not posted:
Confidence changes required:33%
None
4. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:240
- Draft comment:
The header area now appends three containers (composite bar, global header, custom title bar) whose visual order depends on the CSS flex 'order' properties. Ensure that the CSS ordering (order: 1 for .auxiliary-bar-global-header and order: 2 for .custom-title-container) aligns with the intended UI layout. A short inline comment could clarify the intended stacking order. - Reason this comment was not posted:
Confidence changes required:33%
None
5. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryTitleBar.ts:34
- Draft comment:
Usingicon.id
to build the button’s class name works if all icon objects define an 'id'. Consider ensuring that every icon in 'iconLabels' has a valid id. Also, for better accessibility, consider adding appropriate ARIA attributes (e.g. aria-label) to the buttons. - Reason this comment was not posted:
Comment did not seem useful.
6. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryTitleBar.ts:47
- Draft comment:
The dispose method correctly removes the container element from the DOM. If there's any chance of dispose being called more than once, a null-check before calling remove() might help prevent errors. - Reason this comment was not posted:
Confidence changes required:33%
None
7. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:147
- Draft comment:
The style rules for.auxiliary-titlebar
and.auxiliary-custom-titlebar
(and their nested button rules) are nearly identical. Consider refactoring these duplicated rules to a shared class to reduce redundancy and ease future maintenance. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ZFbGxtRqtxIiGaJw
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.
border-bottom: 1px solid var(--vscode-sideBar-border); | ||
} | ||
|
||
.monaco-workbench .part.auxiliarybar .auxiliary-titlebar { |
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.
Styles for .auxiliary-titlebar and .auxiliary-custom-titlebar are nearly identical. Consider refactoring to reduce duplication.
o i c nice |
Important
Introduces a new header bar in
AuxiliaryBarPart
with a customAuxiliaryTitleBar
and updates styles inauxiliaryBarPart.css
.AuxiliaryBarPart
inauxiliaryBarPart.ts
.AuxiliaryTitleBar
class inauxiliaryTitleBar.ts
for custom title bar with command buttons.auxiliaryBarPart.css
to style the new header and title bar, including button hover and active states.This description was created by for f64f65b. It will automatically update as commits are pushed.