jss icon indicating copy to clipboard operation
jss copied to clipboard

1834 link component is not checking if href is undefined

Open sumon1991 opened this issue 1 year ago • 3 comments

  • [x] This PR follows the Contribution Guide
  • [x] Changelog updated
  • [ ] Upgrade guide entry added

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

  • [x] Unit Test Added
  • [ ] Manual Test/Other (Please elaborate)

Types of changes

  • [x] 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)

sumon1991 avatar Jul 25 '24 21:07 sumon1991

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:

  • https://github.com/Sitecore/jss/blob/dev/packages/sitecore-jss-nextjs/src/components/Link.test.tsx#L508-L536
  • https://github.com/Sitecore/jss/blob/dev/packages/sitecore-jss-react/src/components/Link.test.tsx#L222-L267

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.

pzi avatar Jul 31 '24 07:07 pzi

@pzi do I need to update the test cases?

sumon1991 avatar Jul 31 '24 19:07 sumon1991

@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.

pzi avatar Aug 01 '24 05:08 pzi

After some investigation: the link.href is already being verified in here: https://github.com/Sitecore/jss/blob/85ec232ed9a7928c6066f3abbc03f80d1f89afcb/packages/sitecore-jss-react/src/components/Link.tsx#L50

This is why unit tests are not failing, if href is not there, we consider the link field empty. I believe that change went live around the same time you posted the issue and the PR @sumon1991 , as a part of unrelated feature.

I'll close the PR and the related issue, as, in the end, the issue should no longer be present - but please do reopen the issue and the PR if you still encounter the problem.

art-alexeyenko avatar Apr 23 '25 02:04 art-alexeyenko