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.

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

fix: set scrolled attribute only when offset is larger than the header #2133

Merged
merged 2 commits into from
Sep 22, 2023
Merged

fix: set scrolled attribute only when offset is larger than the header #2133

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90bada6
fix: set scrolled attribute only when offset is larger than the header
Sep 20, 2023
0b0c9c3
fix: comment hardcoded value and add todo
Sep 21, 2023
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
4 changes: 3 additions & 1 deletion packages/components/src/components/telekom/telekom-header/telekom-header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export class TelekomHeader {

@Listen('scroll', { target: 'document' })
onScroll() {
this.scrolled = window.pageYOffset > 2;
// 48px is the height of the header, set scrolled when the user scrolls past it
// todo: calculate this value dynamically (for slim header, smaller viewports, etc)
this.scrolled = window.pageYOffset > 48;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer this 48 not be hard-coded, but calculated… if not possible (or too complex), at least add a comment explaining where the number is coming from 🤓

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i thought about calculating it here but it seemed rather overkill for this. Only issue i see with this now, is that on medium viewport the header is not 48px but a little bit smaller - IMO not a problem, it would just minimize when scrolling 10px more than the header height at that viewport size. (or was it requested by UX/UI that the header minimizes ASAP? )

this.scrolledBack =
this.pageYOffset !== window.pageYOffset && window.pageYOffset <= 0;
this.pageYOffset = pageYOffset;
Expand Down
Loading