Skip to content

feat(component): implemented breakpoints utlity in the post-header component #4652

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

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

feat(component): implemented breakpoints utlity in the post-header component #4652

wants to merge 24 commits into from

Conversation

alionazherdetska
Copy link
Contributor

@alionazherdetska alionazherdetska commented Feb 11, 2025 •
edited
Loading

No description provided.

@alionazherdetska alionazherdetska requested a review from a team as a code owner February 11, 2025 15:27
@alionazherdetska alionazherdetska linked an issue Feb 11, 2025 that may be closed by this pull request
Use the exising breakpoints utlity in the new composible header #4527
Closed
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 11, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 07738fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@swisspost/design-system-components Patch
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components-react Patch
@swisspost/design-system-documentation Patch
@swisspost/design-system-components-angular Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Patch
@swisspost/design-system-tokens Patch
@swisspost/design-system-intranet-header Patch
@swisspost/design-system-icons Patch
@swisspost/design-system-styles-primeng Patch
@swisspost/internet-header Patch
@swisspost/design-system-intranet-header-workspace Patch
@swisspost/design-system-styles-primeng-workspace Patch
@swisspost/design-system-intranet-header-showcase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Feb 11, 2025 •
edited
Loading

Related Previews

@leagrdv leagrdv added the Header Everything related to our composible header components label Feb 20, 2025
@alionazherdetska alionazherdetska force-pushed the 4527-use-the-exising-breakpoints-utlity-in-the-new-composible-header branch from c634e57 to 52d2081 Compare February 21, 2025 08:27
leagrdv
leagrdv requested changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@leagrdv leagrdv left a comment

Choose a reason for hiding this comment

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

The calls seem to be made at the right time now, but on scroll the header height is not reduced anymore in desktop

@alionazherdetska alionazherdetska force-pushed the 4527-use-the-exising-breakpoints-utlity-in-the-new-composible-header branch from 0aaebc0 to 9e3448a Compare February 21, 2025 13:37
@oliverschuerch oliverschuerch self-requested a review February 24, 2025 13:49
@oliverschuerch
Copy link
Contributor

Depends on #4764

@oliverschuerch oliverschuerch added the 🚂 PR train PR which follows another one. label Feb 24, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

oliverschuerch
oliverschuerch requested changes Feb 25, 2025
View reviewed changes
packages/components/src/components/post-header/post-header.tsx
*/
@Event() postUpdateDevice: EventEmitter<DEVICE_SIZE>;
connectedCallback() {
window.addEventListener('postBreakpoint:name', this.breakpointChange.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this.breakpointChange is a private function of the component class (getting the context of the class anyway), do we really need to bind(this) here?

Make sure you update the removeEventListener respectively!

packages/components/src/components/post-header/post-header.tsx
@Event() postUpdateDevice: EventEmitter<DEVICE_SIZE>;
connectedCallback() {
window.addEventListener('postBreakpoint:name', this.breakpointChange.bind(this));
window.addEventListener('resize', this.throttledResize, { passive: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

this.thottledScroll is not thorttling at all and only returns a function call.
Can be simplified by:

Suggested change
window.addEventListener('resize', this.throttledResize, { passive: true });
window.addEventListener('resize', this.handleScrollEvent, { passive: true });

Make sure you update the removeEventListener respectively!
And you can then remove the line above private throttledScroll = () => this.handleScrollEvent();

BTW: This event gets binded twice in the component.
Once in the componentWillRender and once in the connectedCallback lifecylce hook.
I would suggest to remove the binding in the componentWillRender hook and only keep the other.

packages/components/src/components/post-header/post-header.tsx
this.switchLanguageSwitchMode();
});
}
this.device = breakpoint.get('name');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary, because you're setting this.device initially and then everytime a breakpointChange happens.

packages/components/src/components/post-megadropdown/post-megadropdown.tsx
this.device = event.detail;
});
}
window.addEventListener('postBreakpoint:name', this.breakpointChange.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the context binding (.bind(this)) really necessary?
If not, remove it and update the removeEventListener respectively.

@gfellerph gfellerph added the on hold Wait for something to be done, before continue working on this Issue label Feb 25, 2025
@gfellerph
Copy link
Member

on hold until cargo go live

@gfellerph gfellerph added needs: 🏓 dev roundtable To be discussed at the roundtable of esteemed developers labels Apr 22, 2025
@alionazherdetska alionazherdetska mentioned this pull request Apr 28, 2025
Open
@alionazherdetska
Copy link
Contributor Author

After discussion in the dev roundtable, we've decided to close this PR and create a new ticket: #5357.

Reasons:
This PR has been on hold for an extended period
There are now several merge conflicts that would be time-consuming to resolve
It would be more efficient to implement this from scratch with the current codebase

@github-project-automation github-project-automation bot moved this from 👀 Triage to 🚀 Done in Design System Production Board Apr 28, 2025
@gfellerph gfellerph removed the needs: 🏓 dev roundtable To be discussed at the roundtable of esteemed developers label Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverschuerch oliverschuerch oliverschuerch requested changes

@leagrdv leagrdv Awaiting requested review from leagrdv

Assignees
No one assigned
Labels
Header Everything related to our composible header components on hold Wait for something to be done, before continue working on this Issue 🚂 PR train PR which follows another one.
Projects
Design System Production Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use the exising breakpoints utlity in the new composible header
5 participants
@alionazherdetska @swisspost-bot @oliverschuerch @gfellerph @leagrdv