Skip to content

Conversation

derek-cmsds
Copy link
Collaborator

@derek-cmsds derek-cmsds commented Sep 26, 2025

Summary

  • The learnMore prop is optional, however the text/link that the prop applies was displaying whether the prop is applied or not. This PR updates the "Learn more" link to only display when learnMoreUrl is defined.
  • CMSDS-3527

How to test

  1. Pull down the branch and run npm start
  2. Navigate to the default ThirdPartyExternalLink story in Storybook
  3. Click the link and verify the modal appears
  4. Verify the modal includes a link with the text "Learn more about links to third-party sites."
  5. In the controls section, remove the learnMoreUrl control value.
  6. Verify the "Learn more about links to third-party sites." link disappears.

Checklist

  • Prefixed the PR title with the Jira ticket number as [CMSDS-####] Title or [NO-TICKET] if this is unticketed work.
  • Selected appropriate Type (only one) label for this PR, if it is a breaking change, label should only be Type: Breaking
  • Selected appropriate Impacts, multiple can be selected.
  • Selected appropriate release milestone

If this is a change to code:

  • Created or updated unit tests to cover any new or modified code
  • Verified that running both npm run test:unit and npm run test:browser:all were each successful
  • If necessary, updated unit-test snapshots (npm run test:unit:update) and browser-test snapshots (npm run test:browser:all:update)

@derek-cmsds derek-cmsds changed the title [CMSDS-3527] Only render learn more link when learnMoreUrl is defined [CMSDS-3527] Only render "learn more link when learnMoreUrl is defined Sep 26, 2025
@derek-cmsds derek-cmsds changed the title [CMSDS-3527] Only render "learn more link when learnMoreUrl is defined [CMSDS-3527] Only render "Learn more" link when learnMoreUrl is defined Sep 26, 2025
@derek-cmsds derek-cmsds added Type: Fixed Indicates an unexpected problem or unintended behavior Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. labels Sep 26, 2025
@derek-cmsds derek-cmsds added this to the 13.1.0 milestone Sep 26, 2025
@derek-cmsds derek-cmsds marked this pull request as ready for review September 26, 2025 19:36
Copy link
Collaborator

@tamara-corbalt tamara-corbalt left a comment

Choose a reason for hiding this comment

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

Looks great! Added two non-blocking suggestions

expect(learnMoreLink.getAttribute('href')).toBe('https://www.google.com/');
});

describe('when no learnMoreUrl is defined', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test! I like that this is scoped inside a dedicated describe block—it adds clarity. We might want to consider more granular groupings like this for better readability across our test suites.

NB: If there are other tests related to this prop, maybe we can generalize the describe name a bit and move them into this block too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

NB: Just a heads up: we may also want to add this test for the ds-third-party-link web component. Unfortunately, that component doesn’t exist on main yet, but something to keep in mind once it's merged.

Copy link
Collaborator

@jack-ryan-nava-pbc jack-ryan-nava-pbc left a comment

Choose a reason for hiding this comment

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

Tested locally with the Core version. This looks good to me. Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Fixed Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants