Skip to content
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

Make the sidebar fill the vertical space. #3864

Merged
merged 5 commits into from May 14, 2024

Conversation

jyasskin
Copy link
Collaborator

@jyasskin jyasskin commented May 8, 2024

Also remove a margin-top from the footer that was creating a gap with the sidebar in short windows. Then the footer's shadow disappeared because the z-index was being ignored, which required adding a position property. It's possible we'll want the #content-flex-wrapper to include some padding so things don't run right up to the footer, but nothing I've tested looks particularly bad.

Fixes #3852.

Most of the changed lines are unindenting a block after removing one <div> layer: look at the diff without whitespace changes.

Short Window Tall Window
Before image image
After image image

@jyasskin jyasskin requested a review from KyleJu May 8, 2024 22:25
Copy link
Collaborator

@KyleJu KyleJu left a comment

Choose a reason for hiding this comment

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

LGTM. Would you mind including a screenshot after the change?

@KyleJu
Copy link
Collaborator

KyleJu commented May 8, 2024

Also Playwright Tests need to be regenerated

@jyasskin
Copy link
Collaborator Author

jyasskin commented May 8, 2024

@dlaliberte, this change shifts the packages/playwright/tests/__screenshots__/chromedash-guide-feature-page_pwtest.js/add-an-origin-trial-stage/create-origin-trial-stage-dialog-* screenshots so that the mask covers a different part of the dialog under test. Is the relevant part of the dialog still visible so that it's being tested?

@jyasskin jyasskin requested a review from dlaliberte May 8, 2024 23:40
@dlaliberte
Copy link
Collaborator

@dlaliberte, this change shifts the packages/playwright/tests/__screenshots__/chromedash-guide-feature-page_pwtest.js/add-an-origin-trial-stage/create-origin-trial-stage-dialog-* screenshots so that the mask covers a different part of the dialog under test. Is the relevant part of the dialog still visible so that it's being tested?

Answered in chat earlier, but here is a bit more detail. The top part of the dialog popup (dropdown) is covered by the mask, and it would be better to arrange that this didn't happen, somehow, if not too difficult. Perhaps by scrolling the window content, resizing the window, or creating the mask in a way that is below the dialog. Or maybe the screenshot could just focus on the popup element. Not a big deal in any event.

Also remove a margin-top from the footer that was
creating a gap with the sidebar in short windows.
Then the footer's shadow disappeared because the
z-index was being ignored, which required adding a
position property. It's possible we'll want the
content-flex-wrapper to include some padding so
things don't run right up to the footer, but
nothing I've tested looks particularly bad.
@jyasskin
Copy link
Collaborator Author

Expanding the incubation section moved the history section far enough away that its mask doesn't cover the OT dialog. Thanks for the reviews; I'll merge after the tests pass.

@jyasskin jyasskin merged commit c001d4e into GoogleChrome:main May 14, 2024
7 checks passed
@jyasskin jyasskin deleted the short-sidebar branch May 14, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The sidebar in the external reviews page doesn't use the full window height
3 participants