-
-
Notifications
You must be signed in to change notification settings - Fork 13.2k
๐ style: Fix win electron style #8439
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR restructures the Windows title bar component by replacing detailed styles and interactive markup with a static placeholder, enhances Electron Browser window creation by introducing a titleBarOverlay, streamlining background material configuration, and adding robust visual effects methods, and refactors tooltip integration by consolidating Tooltip props directly into ActionIcon components. Class diagram for updated Electron Browser visual effects methodsclassDiagram
class Browser {
+applyVisualEffects()
+reapplyVisualEffects()
}
Browser : -_browserWindow
Browser : +retrieveOrInitialize()
Browser : +getWindowsVersion()
Browser : +identifier
Browser : +logger
Browser : +applyVisualEffects() // now checks isDestroyed and uses try/catch
Browser : +reapplyVisualEffects() // new method, calls applyVisualEffects if Windows 11
Class diagram for WinControl component simplificationclassDiagram
class WinControl {
+render()
}
%% Previously: used useStyles, interactive window controls
%% Now: returns static <div style={{ width: 132 }} />
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The latest updates on your projects. Learn more about Vercel for Git โ๏ธ
|
๐ @canisminor1990 Thank you for raising your pull request and contributing to our Community |
TestGru AssignmentSummary
Tip You can |
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.
Hey @canisminor1990 - I've reviewed your changes - here's some feedback:
- Remove large blocks of commented-out code in WinControl and Browser or extract them behind a feature flag to keep the implementation clear and maintainable.
- Annotate or add a TODO explaining why WinControl now only renders a fixed-width so future readers understand the placeholder intent.
- Consider syncing the hardcoded titleBarOverlay.height (30) with your existing TITLE_BAR_HEIGHT constant to avoid mismatched layouts on Windows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove large blocks of commented-out code in WinControl and Browser or extract them behind a feature flag to keep the implementation clear and maintainable.
- Annotate or add a TODO explaining why WinControl now only renders a fixed-width <div> so future readers understand the placeholder intent.
- Consider syncing the hardcoded titleBarOverlay.height (30) with your existing TITLE_BAR_HEIGHT constant to avoid mismatched layouts on Windows.
## Individual Comments
### Comment 1
<location> `src/features/ElectronTitlebar/WinControl/index.tsx:56` </location>
<code_context>
- </div>
- </div>
- );
+ return <div style={{ width: 132 }} />;
+
+ // const { styles } = useStyles();
</code_context>
<issue_to_address>
Returning a static width div may impact layout and accessibility.
If this is a temporary placeholder, add a clear TODO or use a more descriptive placeholder to prevent confusion for users and maintainers.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return <div style={{ width: 132 }} />;
// const { styles } = useStyles();
=======
// TODO: Replace this placeholder with the actual window control implementation.
// This is a temporary placeholder to maintain layout.
return (
<div
style={{ width: 132, height: 24, display: 'flex', alignItems: 'center', justifyContent: 'center', background: 'transparent' }}
role="presentation"
aria-hidden="true"
tabIndex={-1}
data-testid="wincontrol-placeholder"
>
{/* Window controls will be implemented here */}
<span style={{ fontSize: 12, color: '#888' }}>Window Controls Placeholder</span>
</div>
);
// const { styles } = useStyles();
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/features/ElectronTitlebar/WinControl/index.tsx:1` </location>
<code_context>
+// const useStyles = createStyles(({ css, cx, token }) => {
</code_context>
<issue_to_address>
Commenting out the entire styling logic may lead to inconsistent UI.
If this change is temporary, confirm that the component and application still function correctly without these styles.
</issue_to_address>
### Comment 3
<location> `apps/desktop/src/main/core/Browser.ts:249` </location>
<code_context>
- }
- : {}),
autoHideMenuBar: true,
- backgroundColor: '#00000000',
+
frame: false,
</code_context>
<issue_to_address>
Removing the transparent background color may affect window appearance.
Please verify that removing backgroundColor does not result in an unintended opaque window or loss of transparency.
</issue_to_address>
### Comment 4
<location> `apps/desktop/src/main/core/Browser.ts:439` </location>
<code_context>
- this._browserWindow?.setBackgroundMaterial(isDarkMode ? 'mica' : 'acrylic');
- this._browserWindow?.setVibrancy('under-window');
+
+ try {
+ // Apply background material
+ this._browserWindow.setBackgroundMaterial('mica');
+ logger.debug(
+ `[${this.identifier}] Visual effects applied successfully (dark mode: ${isDarkMode})`,
+ );
+ } catch (error) {
+ logger.error(`[${this.identifier}] Failed to apply visual effects:`, error);
+ }
</code_context>
<issue_to_address>
Catching errors when applying visual effects increases stability.
Consider if errors should be communicated to the user or if a retry mechanism is appropriate, rather than only logging them.
Suggested implementation:
```typescript
const applyEffects = (retry = false) => {
try {
// Apply background material
this._browserWindow.setBackgroundMaterial('mica');
logger.debug(
`[${this.identifier}] Visual effects applied successfully (dark mode: ${isDarkMode})`,
);
} catch (error) {
logger.error(`[${this.identifier}] Failed to apply visual effects${retry ? ' (retry)' : ''}:`, error);
if (!retry) {
// Retry once after a short delay
setTimeout(() => applyEffects(true), 500);
} else {
// Notify user after retry fails
if (typeof showErrorNotification === 'function') {
showErrorNotification('Failed to apply visual effects to the window. Some UI features may not display as intended.');
}
}
}
};
applyEffects();
```
- This code assumes a `showErrorNotification` function is available in scope to notify the user. If your app uses a different notification system, replace `showErrorNotification` with the appropriate function.
- If you want to customize the retry logic (e.g., more retries, different delay), adjust the `applyEffects` function accordingly.
</issue_to_address>
Help me be more useful! Please click ๐ or ๐ on each comment and I'll use the feedback to improve your reviews.
return <div style={{ width: 132 }} />; | ||
|
||
// const { styles } = useStyles(); |
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.
suggestion: Returning a static width div may impact layout and accessibility.
If this is a temporary placeholder, add a clear TODO or use a more descriptive placeholder to prevent confusion for users and maintainers.
return <div style={{ width: 132 }} />; | |
// const { styles } = useStyles(); | |
// TODO: Replace this placeholder with the actual window control implementation. | |
// This is a temporary placeholder to maintain layout. | |
return ( | |
<div | |
style={{ width: 132, height: 24, display: 'flex', alignItems: 'center', justifyContent: 'center', background: 'transparent' }} | |
role="presentation" | |
aria-hidden="true" | |
tabIndex={-1} | |
data-testid="wincontrol-placeholder" | |
> | |
{/* Window controls will be implemented here */} | |
<span style={{ fontSize: 12, color: '#888' }}>Window Controls Placeholder</span> | |
</div> | |
); | |
// const { styles } = useStyles(); |
// const useStyles = createStyles(({ css, cx, token }) => { | ||
// const icon = css` | ||
// display: flex; | ||
// align-items: center; | ||
// justify-content: center; | ||
// | ||
// width: ${TITLE_BAR_HEIGHT * 1.2}px; | ||
// min-height: ${TITLE_BAR_HEIGHT}px; | ||
// | ||
// color: ${token.colorTextSecondary}; |
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.
issue (bug_risk): Commenting out the entire styling logic may lead to inconsistent UI.
If this change is temporary, confirm that the component and application still function correctly without these styles.
autoHideMenuBar: true, | ||
backgroundColor: '#00000000', |
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.
issue (bug_risk): Removing the transparent background color may affect window appearance.
Please verify that removing backgroundColor does not result in an unintended opaque window or loss of transparency.
try { | ||
// Apply background material | ||
this._browserWindow.setBackgroundMaterial('mica'); | ||
logger.debug( | ||
`[${this.identifier}] Visual effects applied successfully (dark mode: ${isDarkMode})`, | ||
); | ||
} catch (error) { |
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.
suggestion: Catching errors when applying visual effects increases stability.
Consider if errors should be communicated to the user or if a retry mechanism is appropriate, rather than only logging them.
Suggested implementation:
const applyEffects = (retry = false) => {
try {
// Apply background material
this._browserWindow.setBackgroundMaterial('mica');
logger.debug(
`[${this.identifier}] Visual effects applied successfully (dark mode: ${isDarkMode})`,
);
} catch (error) {
logger.error(`[${this.identifier}] Failed to apply visual effects${retry ? ' (retry)' : ''}:`, error);
if (!retry) {
// Retry once after a short delay
setTimeout(() => applyEffects(true), 500);
} else {
// Notify user after retry fails
if (typeof showErrorNotification === 'function') {
showErrorNotification('Failed to apply visual effects to the window. Some UI features may not display as intended.');
}
}
}
};
applyEffects();
- This code assumes a
showErrorNotification
function is available in scope to notify the user. If your app uses a different notification system, replaceshowErrorNotification
with the appropriate function. - If you want to customize the retry logic (e.g., more retries, different delay), adjust the
applyEffects
function accordingly.
Codecov ReportAll modified and coverable lines are covered by tests โ
Additional details and impacted files@@ Coverage Diff @@
## main #8439 +/- ##
=========================================
Coverage 85.31% 85.31%
=========================================
Files 908 908
Lines 68519 68519
Branches 6514 4431 -2083
=========================================
Hits 58455 58455
Misses 10064 10064
Flags with carried forward coverage won't be shown. Click here to find out more. โ View full report in Codecov by Sentry. ๐ New features to boost your workflow:
|
8c3f72f
to
7fda6b6
Compare
7fda6b6
to
34f3fbe
Compare
๐ป ๅๆด็ฑปๅ | Change Type
๐ ๅๆด่ฏดๆ | Description of Change
electron-window-state
ไผๅ็ชๅฃ้ป่พTODO
๐ ่กฅๅ ไฟกๆฏ | Additional Information