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

[ui] Affix the page header to the top of the page #17783

Merged
merged 1 commit into from Jun 30, 2023

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Jun 30, 2023

A report shows a 3rd party browser extension puts a banner at the top of page and awkwardly shifts nav; this fixes that

image

@github-actions
Copy link

github-actions bot commented Jun 30, 2023

Ember Test Audit comparison

main 6b0cfc7 change
passes 1494 1494 0
failures 1 1 0
flaky 0 0 0
duration 000ms 000ms -000ms

@philrenaud philrenaud requested a review from tgross June 30, 2023 20:04
@philrenaud philrenaud changed the title top-fix nav/page header [ui] Affix the page header to the top of the page Jun 30, 2023
@@ -12,6 +12,7 @@
position: fixed;
width: 100%;
z-index: $z-header;
top: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little context for this: this is a position: fixed element, which means that you can scroll your browser generally, and wherever this element is when it started, it'll remain there. Perfect for headers, footers, etc.

A position: <anything but static> element's top, bottom, right or left properties will affect how near it is to some container. for position: absolute, this is the distance from the closest parent element with relative positioning. For position: fixed like this one, this is the distance from the browser's border.

As such, top: 0 indicates that it should always start at the very upper edge of the browser. Which it was doing already, unless some extension slots in before it to bump it down a few pixels. This just says "Even if that happens, start at y=0"

@philrenaud philrenaud marked this pull request as ready for review June 30, 2023 20:09
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

…f page and awkwardly shifts nav; this fixes that
@philrenaud philrenaud force-pushed the f-ui/account-for-browser-extension-pushed-headers branch from 4ad90d9 to 6b0cfc7 Compare June 30, 2023 20:45
@philrenaud philrenaud merged commit 17f63cf into main Jun 30, 2023
14 checks passed
@philrenaud philrenaud deleted the f-ui/account-for-browser-extension-pushed-headers branch June 30, 2023 21:09
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.

None yet

2 participants