NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

fix: Remove CSS overrides for material-ui#1444

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

kylecarbs wants to merge1 commit intomainfromrmoverrides

Closed

fix: Remove CSS overrides for material-ui#1444

kylecarbs wants to merge1 commit intomainfromrmoverrides

Conversation

Copy link

Member

kylecarbs commentedMay 13, 2022

This hopefully removes some debt, and doesn't mess up
styling too much!

fix: Remove CSS overrides for material-ui

abde75b

This hopefully removes some debt, and doesn't mess upstyling too much!

kylecarbs self-assigned this

May 13, 2022

kylecarbs requested a review froma team as acode owner

May 13, 2022 22:32

Copy link

codecovbot commentedMay 13, 2022•
edited
Loading

Codecov Report

Merging#1444 (abde75b) intomain (50ad2f8) willincrease coverage by0.01%.
The diff coverage isn/a.

@@            Coverage Diff             @@##             main    #1444      +/-   ##==========================================+ Coverage   67.10%   67.12%   +0.01%==========================================  Files         291      290       -1       Lines       19527    19518       -9       Branches      258      256       -2     ==========================================- Hits        13104    13101       -3+ Misses       5073     5070       -3+ Partials     1350     1347       -3

Flag	Coverage Δ
unittest-go-macos-latest	`54.23% <ø> (-0.07%)`	⬇️
unittest-go-postgres-	`65.62% <ø> (+0.11%)`	⬆️
unittest-go-ubuntu-latest	`56.56% <ø> (+0.01%)`	⬆️
unittest-go-windows-2022	`52.64% <ø> (-0.01%)`	⬇️
unittest-js	`74.62% <ø> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
site/src/theme/theme.ts	`100.00% <ø> (ø)`
provisionersdk/serve.go	`35.13% <0.00%> (-8.11%)`	⬇️
peer/conn.go	`78.86% <0.00%> (-2.95%)`	⬇️
cli/cliui/provisionerjob.go	`76.42% <0.00%> (-2.15%)`	⬇️
provisionerd/provisionerd.go	`78.04% <0.00%> (+0.53%)`	⬆️
coderd/provisionerdaemons.go	`64.48% <0.00%> (+1.17%)`	⬆️
agent/agent.go	`67.76% <0.00%> (+1.23%)`	⬆️
provisioner/echo/serve.go	`56.80% <0.00%> (+2.39%)`	⬆️
peerbroker/listen.go	`87.39% <0.00%> (+3.36%)`	⬆️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update50ad2f8...abde75b. Read thecomment docs.

greyscaled mentioned this pull request

May 14, 2022

Remove theme overrides#1449

Closed

greyscaled suggested changes

May 14, 2022

View reviewed changes

Copy link

Contributor

greyscaled left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This breaks a few things, and I feel that it overlaps with#803 (TL:DR - Adjusting the theme to be dark only, including adjustments that are needed in all pages/components).

Since there's a lot of pieces here, and it's not on CE scope to remove the overrides, I suggest we use the chromatic build here as a reference point and hand-off to@Kira-Pilot for#803 and its concerns.

The goal can be to solve these incrementally (whether it's with#803 or after, so as to avoid doubling up on style work or creating conflict) for minimal risk.

I captured these thoughts in#1449