Skip to content

Refactor/rm computer monitor #2088

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: gh-pages
Choose a base branch
from
Open

Refactor/rm computer monitor #2088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42b137f
refactor: remove the computer monitor background image since it feels…
RafaelJohn9 Sep 23, 2025
52da97c
refactor: changed colors from various html tags inside the monitor ta…
RafaelJohn9 Sep 23, 2025
8197b8f
chore: removed the monitor assets since they aren't needed anymore
RafaelJohn9 Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 4 additions & 19 deletions assets/sass/front-page.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ $monitor-height: 271px;
}

.monitor {
@include background-image-2x($baseurl + "images/monitor-default", $monitor-width, $monitor-height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also remove the assets if we don't need them no more.

Copy link
Member

Choose a reason for hiding this comment

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

BTW this is also the reason for the Playwright test failure:

before after
0b8e1c7b4a7e58b23d502940afdb317184e893e8 1592a2271140f8c85e0e8dca3ad09626c2c91847

Notice the upper part of the monitor in the left screenshot?

Copy link
Member

Choose a reason for hiding this comment

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

BTW originally I had meant not to compare screenshots, but to overlay with a really blurry <div> and then measure the pixel brightness, then verify that it is relatively bright for light mode and dark for dark mode. But IIRC I got stuck somewhere along the lines. Maybe it was the "measure pixel brightness".

Copy link
Member

Choose a reason for hiding this comment

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

Whoa, I can't believe that I couldn't make it work back then. It's relatively easy:

const { chromium } = require('playwright');
const fs = require('fs');

(async () => {
  const browser = await chromium.launch(/* uncomment for debugging: { headless: false } */);
  const page = await browser.newPage();
  await page.goto('https://git-scm.com');

  const getBrightness = async () => {
    // Take screenshot as base64
    const screenshot = await page.screenshot({ type: 'png' });
    return await page.evaluate(async (base64) => {
      return new Promise((resolve) => {
        const img = new Image();
        img.src = 'data:image/png;base64,' + base64;
        img.onload = () => {
          const canvas = document.createElement('canvas');
          canvas.width = canvas.height = 1;
          const ctx = canvas.getContext('2d');

          // Draw scaled-down image to average colors
          ctx.drawImage(img, 0, 0, 1, 1);
          const [r, g, b] = ctx.getImageData(0, 0, 1, 1).data;

          // Calculate brightness, for more details, see
          // https://en.wikipedia.org/wiki/Relative_luminance#Relative_luminance_and_%22gamma_encoded%22_colorspaces
          const brightness = 0.2126 * r + 0.7152 * g + 0.0722 * b;
          resolve(brightness);
        };
      });
    }, screenshot.toString('base64'));
  };

  console.log(`Estimated brightness: ${await getBrightness()}`);

  const darkModeButton = page.locator('#dark-mode-button');
  await darkModeButton.click();

  console.log(`Estimated dark brightness: ${await getBrightness()}`);

  // Inject image into page and analyze brightness
  await browser.close();
})();

For me, this prints:

Estimated brightness: 216.4316
Estimated dark brightness: 63.005199999999995

Therefore, I think the best idea would be to change the Playwright tests that want to verify that dark mode is dark, light mode is bright, with a variation of this code, say, dividing the return value by 255 to limit it to the range between 0 and 1, and then assert something like expect(await getBrightness()).toBeCloseTo(0.25, 2) for dark mode (and 0.85 for light mode).

@RafaelJohn9 do you want to run with this and replace the screenshot-based assertions (and likewise these assertions) with this kind of more robust approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing 👍,

Will update the tests 🤝

Copy link
Member

Choose a reason for hiding this comment

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

@RafaelJohn9 maybe that update should be split out into its own PR, which could then be fast-tracked?

Copy link
Contributor Author

@RafaelJohn9 RafaelJohn9 Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ooh okay,

lemme reset 👍

Copy link
Member

Choose a reason for hiding this comment

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

I didn't mean the deletion of the resources that are no longer referenced. I meant the test update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

misunderstood that 😅 ,

Awesome, so should I open another PR referencing the same issue, or should I create a separate issue for it ?

Copy link
Member

@dscho dscho Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In git-scm.com, we play a bit looser (albeit not as loose as the Git mailing list): We don't require an issue for every PR. Just open another PR with the updated test (and removing the stored screenshots), maybe referencing this finding about the Playwright tests being broken by benign updated as justification.

width: $monitor-width - 40;
height: $monitor-height - 45;
padding-top: 45px;
Expand All @@ -137,7 +136,7 @@ $monitor-height: 271px;

h4 {
font-weight: normal;
color: var(--main-bg);
color: var(--orange);
font-size: 16px;
}

Expand All @@ -146,16 +145,18 @@ $monitor-height: 271px;
margin-bottom: 6px;
font-size: 28px;
font-weight: bold;
color: #eee;
}

a {
color: #eee;
color: var(--font-color);
font-size: 12px;
text-decoration: underline;
}

span.release-date {
font-size: 12px;
color: #eee;
}

a.button {
Expand All @@ -181,22 +182,6 @@ $monitor-height: 271px;
background-image: linear-gradient(#1a7e84, #165b60);
}
}

&.mac {
@include background-image-2x($baseurl + "images/monitor-mac", $monitor-width, $monitor-height);
width: $monitor-width - 40;
height: $monitor-height - 36;
padding-top: 36px;
padding-left: 40px;
}

&.windows {
@include background-image-2x($baseurl + "images/monitor-windows", $monitor-width, $monitor-height);
}

&.linux {
@include background-image-2x($baseurl + "images/monitor-linux", $monitor-width, $monitor-height);
}
}

@property --floor {
Expand Down
Binary file removed static/images/monitor-default.png
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/[email protected]
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/monitor-linux.png
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/[email protected]
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/monitor-mac.png
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/[email protected]
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/monitor-windows.png
View file
Open in desktop
Binary file not shown.
Binary file removed static/images/[email protected]
View file
Open in desktop
Binary file not shown.
Loading