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

1834 link component is not checking if href is undefined #1856

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

sumon1991
Copy link

@sumon1991 sumon1991 commented Jul 25, 2024

Description / Motivation

Describe the Bug
If the field passed to the Link component has an undefined href, it will render undefined as a string in the dom.

import {Link} from '@sitecore-jss/sitecore-jss-nextjs';

const field = {value: {href: undefined, text: 'This is a link with a href that is undefined'}};

<Link field={field} />

This will render a link like this:

<a href="undefined">This is a link with a href that is undefined</a>

Expected Behavior

<a>This is a link with a href that is undefined</a>

1834

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@yavorsk yavorsk added the backlog Issue/PR/discussion is reviewed and added to backlog for the further work label Jul 29, 2024
@pzi
Copy link
Contributor

pzi commented Jul 31, 2024

Thanks for tackling this @sumon1991 🙏🏼

I noticed that there are tests that pass in undefined href values and then it's supposed to render nothing:

Now wondering why they are not failing and/or why the component still rendered with the undefined string value 😕

Probably also worth adding a test to check the fix this PR adds.

@sumon1991
Copy link
Author

sumon1991 commented Jul 31, 2024

@pzi do I need to update the test cases?

@pzi
Copy link
Contributor

pzi commented Aug 1, 2024

@pzi do I need to update the test cases?

Ultimately, it's up to the Sitecore folks to decide that, but I, personally, would add one so it captures that decision to specifically render links without href attributes. Also means it will highlight any regressions if it accidentally gets changed in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants