-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add Material UI 3 component library #3606
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: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Opened this for review, but I am unsure if it's actually ready to get merged. |
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.
Pull Request Overview
This PR integrates the mdui Material Design 3 component library into the YouTube Music desktop application to enable modern UI components on top of the existing interface. The changes focus on making LitElements coexist with the existing Polymer components and resolving CSS conflicts between YouTube Music and mdui.
- Adds mdui library dependency and initialization
- Implements CSS conflict resolution through inline styles and script removal
- Formats existing code for better readability
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/renderer.ts | Adds mdui imports and theme setup, reformats querySelector chains |
src/preload.ts | Implements mdui CSS integration and removes conflicting scripts |
patches/mdui.patch | Patches mdui for SolidJS compatibility instead of React |
package.json | Adds mdui dependency and patch configuration |
assets/mdui.css | Full mdui CSS stylesheet with Material Design 3 tokens |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
import 'mdui'; | ||
import { setTheme } from 'mdui/functions/setTheme.js'; | ||
|
||
setTheme('dark') |
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.
Missing semicolon at the end of the statement. Add a semicolon for consistency with the rest of the codebase.
setTheme('dark') | |
setTheme('dark'); |
Copilot uses AI. Check for mistakes.
|
||
import config from './config'; | ||
|
||
import mduiStyleSheet from "@assets/mdui.css?inline" |
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.
Missing semicolon at the end of the import statement. Add a semicolon for consistency with the rest of the codebase.
import mduiStyleSheet from "@assets/mdui.css?inline" | |
import mduiStyleSheet from "@assets/mdui.css?inline"; |
Copilot uses AI. Check for mistakes.
contextBridge.exposeInMainWorld('litIssuedWarnings', new Set([ | ||
"Lit is in dev mode. Not recommended for production! See https://lit.dev/msg/dev-mode for more information.", | ||
"Shadow DOM is being polyfilled via `ShadyDOM` but the `polyfill-support` module has not been loaded. See https://lit.dev/msg/polyfill-support-missing for more information." | ||
])) |
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.
This code suppresses Lit warnings without explanation. Add a comment explaining why these specific warnings are being suppressed and the implications for the application.
Copilot uses AI. Check for mistakes.
import 'mdui'; | ||
import { setTheme } from 'mdui/functions/setTheme.js'; | ||
|
||
setTheme('dark') |
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.
Should setTheme
be called here?
import 'mdui'; | ||
import { setTheme } from 'mdui/functions/setTheme.js'; | ||
|
||
setTheme('dark') |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Insert ;
setTheme('dark') | |
setTheme('dark'); |
import 'mdui'; | ||
import { setTheme } from 'mdui/functions/setTheme.js'; | ||
|
||
setTheme('dark') |
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.
🚫 [eslint] <stylistic/semi> reported by reviewdog 🐶
Missing semicolon.
setTheme('dark') | |
setTheme('dark'); |
import type { Icons } from '@/types/icons'; | ||
import type { ComponentProps } from 'solid-js'; | ||
import { type IntrinsicElements as MDUIElements } from 'mdui/jsx.en'; |
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.
🚫 [eslint] <importPlugin/order> reported by reviewdog 🐶mdui/jsx.en
import should occur before type import of @/types/icons
import type { Icons } from '@/types/icons'; | |
import type { ComponentProps } from 'solid-js'; | |
import { type IntrinsicElements as MDUIElements } from 'mdui/jsx.en'; | |
import { type IntrinsicElements as MDUIElements } from 'mdui/jsx.en'; | |
import type { Icons } from '@/types/icons'; | |
import type { ComponentProps } from 'solid-js'; |
|
||
import * as config from './config'; | ||
|
||
import mduiStyleSheet from "@assets/mdui.css?inline" |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace "@assets/mdui.css?inline"
with '@assets/mdui.css?inline';
import mduiStyleSheet from "@assets/mdui.css?inline" | |
import mduiStyleSheet from '@assets/mdui.css?inline'; |
|
||
import * as config from './config'; | ||
|
||
import mduiStyleSheet from "@assets/mdui.css?inline" |
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.
🚫 [eslint] <stylistic/quotes> reported by reviewdog 🐶
Strings must use singlequote.
import mduiStyleSheet from "@assets/mdui.css?inline" | |
import mduiStyleSheet from '@assets/mdui.css?inline' |
|
||
import * as config from './config'; | ||
|
||
import mduiStyleSheet from "@assets/mdui.css?inline" |
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.
🚫 [eslint] <stylistic/semi> reported by reviewdog 🐶
Missing semicolon.
import mduiStyleSheet from "@assets/mdui.css?inline" | |
import mduiStyleSheet from "@assets/mdui.css?inline"; |
|
||
import mduiStyleSheet from "@assets/mdui.css?inline" | ||
|
||
contextBridge.exposeInMainWorld('litIssuedWarnings', new Set([ |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace 'litIssuedWarnings',
with ⏎··'litIssuedWarnings',⏎·
contextBridge.exposeInMainWorld('litIssuedWarnings', new Set([ | |
contextBridge.exposeInMainWorld( | |
'litIssuedWarnings', | |
new Set([ |
import mduiStyleSheet from "@assets/mdui.css?inline" | ||
|
||
contextBridge.exposeInMainWorld('litIssuedWarnings', new Set([ | ||
"Lit is in dev mode. Not recommended for production! See https://lit.dev/msg/dev-mode for more information.", |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace "Lit·is·in·dev·mode.·Not·recommended·for·production!·See·https://lit.dev/msg/dev-mode·for·more·information."
with ··'Lit·is·in·dev·mode.·Not·recommended·for·production!·See·https://lit.dev/msg/dev-mode·for·more·information.'
"Lit is in dev mode. Not recommended for production! See https://lit.dev/msg/dev-mode for more information.", | |
'Lit is in dev mode. Not recommended for production! See https://lit.dev/msg/dev-mode for more information.', |
'ytmusic-player-bar', | ||
) |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace ··'ytmusic-player-bar',⏎········
with >('ytmusic-player-bar'
'ytmusic-player-bar', | |
) | |
>('ytmusic-player-bar') |
.querySelector< | ||
HTMLElement & { updateVolume: (volume: number) => void } | ||
>('ytmusic-player-bar') | ||
.querySelector<HTMLElement & { updateVolume: (volume: number) => void }>( |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace HTMLElement·&·{·updateVolume:·(volume:·number)·=>·void·}>(
with ⏎········HTMLElement·&·{·updateVolume:·(volume:·number)·=>·void·}
.querySelector<HTMLElement & { updateVolume: (volume: number) => void }>( | |
.querySelector< | |
HTMLElement & { updateVolume: (volume: number) => void } |
'ytmusic-player-bar', | ||
) |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace ··'ytmusic-player-bar',⏎······
with >('ytmusic-player-bar'
'ytmusic-player-bar', | |
) | |
>('ytmusic-player-bar') |
.querySelector< | ||
HTMLElement & { onVolumeClick: () => void } | ||
>('ytmusic-player-bar') | ||
.querySelector<HTMLElement & { onVolumeClick: () => void }>( |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace HTMLElement·&·{·onVolumeClick:·()·=>·void·}>(
with ⏎········HTMLElement·&·{·onVolumeClick:·()·=>·void·}
.querySelector<HTMLElement & { onVolumeClick: () => void }>( | |
.querySelector< | |
HTMLElement & { onVolumeClick: () => void } |
'ytmusic-player-bar', | ||
) |
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.
🚫 [eslint] <prettier/prettier> reported by reviewdog 🐶
Replace ··'ytmusic-player-bar',⏎······
with >('ytmusic-player-bar'
'ytmusic-player-bar', | |
) | |
>('ytmusic-player-bar') |
About
This PR does the necessary changes needed in order to use a material 3 component library, specifically mdui.
With Material Design 3 we can easily create beautiful UI on-top of ytmusic, I am mainly working on this as a pre-requisite for #3066 .
Tasks:
Fix ytmusic icons, they just can't render(magically went away)