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

fix(scale-telekom-header): fix no service links breaks component #2319

Closed
wants to merge 1 commit into from

Conversation

niklaswa
Copy link
Contributor

@niklaswa niklaswa commented Jun 10, 2024

When using the scale-telekom-header with a profile menu with no service links and hideLoginSettings set to true, the component will crash as the serviceLinks property is undefined.

image

Example:

<scale-telekom-header type="slim">
    <scale-telekom-nav-list variant="functions" slot="functions" scrolled>
        <scale-telekom-profile-menu
            hide-login-settings="true"
            login-url="/login"
            logged-in="true"
            label="Niklas"
            logout-label="Logout"
            logout-url="/logout"
        ></scale-telekom-profile-menu>
    </scale-telekom-nav-list>
</scale-telekom-header>

This PR changes the serviceLinksEmpty method by introducing a preceding property check to see if the property is defined and then continues with the length check.

Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for marvelous-moxie-a6e2fe ready!

Name Link
🔨 Latest commit 19a6d77
🔍 Latest deploy log https://app.netlify.com/sites/marvelous-moxie-a6e2fe/deploys/66671d10340b22000841e05d
😎 Deploy Preview https://deploy-preview-2319--marvelous-moxie-a6e2fe.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@maomaoZH maomaoZH left a comment

Choose a reason for hiding this comment

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

@niklaswa thanks for PR. LGTM

@maomaoZH
Copy link
Collaborator

@niklaswa would you please check the failed tests before you merge it to main? Thanks!

@niklaswa
Copy link
Contributor Author

@niklaswa would you please check the failed tests before you merge it to main? Thanks!

Not really sure why this git check fails there to be honest. Maybe you can rerun the jobs? Doesn't look like a code issue for me at the moment

@amir-ba
Copy link
Collaborator

amir-ba commented Oct 16, 2024

THis will be closed as it is added as part of 2339

@amir-ba amir-ba closed this Oct 16, 2024
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.

3 participants