Skip to content

feat: add external prop to dropdown menu#648

Merged
QuintonJason merged 3 commits intomainfrom
fix/dropdow-target-blank
Jan 29, 2026
Merged

feat: add external prop to dropdown menu#648
QuintonJason merged 3 commits intomainfrom
fix/dropdow-target-blank

Conversation

@QuintonJason
Copy link
Contributor

Description

Adds support for external and target props to pds-dropdown-menu-item to enable proper external link behavior in dropdown menus.

Problem

The pds-dropdown-menu-item component wasn't passing target or external attributes through to the internal link element in its shadow DOM. When developers set target="_blank" on a dropdown menu item (e.g., for "Preview" links in data tables), the attribute was applied to the host element but ignored by the actual <a> tag inside the shadow root, causing all links to open in the same tab.

Solution

Added two complementary props that pass through to the internal pds-link component:

  • external (boolean) - Simpler API for the common case of opening in new tab (recommended)
  • target (string) - Explicit control for all target values (_blank, _self, _parent, _top)

Both approaches work, with target taking precedence when both are set.

Components Modified

  • pds-dropdown-menu-item: Added external and target props
  • pds-link: Added target prop (already had external)

Fixes DSS-97

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Visit the pds-dropdown-menu page to verify

  • unit tests
  • tested manually

Test Configuration:

  • Pine versions:
  • OS:
  • Browsers:
  • Screen readers:
  • Misc:

Checklist:

If not applicable, leave options unchecked.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Design has QA'ed and approved this PR

@QuintonJason QuintonJason self-assigned this Jan 28, 2026
@netlify
Copy link

netlify bot commented Jan 28, 2026

Deploy Preview for pine-design-system ready!

Name Link
🔨 Latest commit 30ee67e
🔍 Latest deploy log https://app.netlify.com/projects/pine-design-system/deploys/697a7188bba6c70008376e89
😎 Deploy Preview https://deploy-preview-648--pine-design-system.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 project configuration.

@github-actions github-actions bot added bug Something isn't working package: core Changes have been made to the Core package labels Jan 28, 2026
@QuintonJason
Copy link
Contributor Author

@coderabbitai review this pr

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2026

🧩 Analysis chain

@QuintonJason I'll review this PR for you.


🏁 Script executed:

#!/bin/bash

# Get the list of changed files in the PR
gh pr view 648 --json files --jq '.files[] | "\(.path) (\(.additions)+/\(.deletions)-)"'

echo -e "\n--- File changes details ---\n"

# Get the diff for the PR
gh pr diff 648

Length of output: 24141


Let me examine the complete pds-link component to check if the rel attribute is already being handled for security:


🏁 Script executed:

#!/bin/bash

# Get the full pds-link.tsx file to check for rel attribute handling
cat libs/core/src/components/pds-link/pds-link.tsx

echo -e "\n--- Checking for rel attribute usage in tests ---\n"

# Check if tests verify rel attribute
cat libs/core/src/components/pds-link/test/pds-link.spec.tsx 2>/dev/null || echo "Test file not found in diff"

echo -e "\n--- Search for rel attribute usage in pds-link ---\n"

# Search for any rel attribute handling in the component
rg -n "rel=" libs/core/src/components/pds-link/

Length of output: 5766


@QuintonJason I've completed my review of this PR. The implementation is well-structured with good documentation and test coverage. However, I've identified a critical security vulnerability that should be addressed.

🔴 Critical Security Issue

Missing rel attribute for target="_blank" links

The pds-link component doesn't set rel="noopener noreferrer" when opening links in a new tab. This creates a security vulnerability where the opened page has access to window.opener and could:

  • Redirect the original page to a phishing site
  • Access the parent window context
  • Cause performance issues

Recommended Fix:

In libs/core/src/components/pds-link/pds-link.tsx, add the rel attribute:

render() {
  const targetValue = this.target || (this.external ? '_blank' : undefined);
  const showExternalIcon = targetValue === '_blank';
  const relValue = targetValue === '_blank' ? 'noopener noreferrer' : undefined;

  return (
    <a
      class={this.classNames()}
      href={this.href}
      id={this.componentId}
      part="link"
      target={targetValue}
      rel={relValue}
      style={this.setLinkStyles()}
    >
      <slot>{this.href}</slot>
      {showExternalIcon &&
        <pds-icon icon={launch} size={this.fontSize}></pds-icon>
      }
    </a>
  );
}

✅ Positive Observations

  1. Well-structured implementation: Props are correctly typed with TypeScript union types
  2. Clear precedence logic: target prop correctly takes precedence over external
  3. Backward compatibility: The deprecated external prop is maintained
  4. Comprehensive documentation: MDX files include good examples
  5. Good test coverage: Tests verify the new functionality
  6. Consistent API: Both pds-link and pds-dropdown-menu-item follow the same pattern

📝 Additional Recommendations

  1. Add test for rel attribute: Update pds-link.spec.tsx to verify that rel="noopener noreferrer" is set when target="_blank":
it('sets rel="noopener noreferrer" when target is _blank', async () => {
  const { root } = await newSpecPage({
    components: [PdsLink],
    html: `<pds-link href="https://example.com" target="_blank">Link</pds-link>`,
  });

  const linkElement = root?.shadowRoot?.querySelector('a');
  expect(linkElement?.getAttribute('rel')).toBe('noopener noreferrer');
});
  1. Consider accessibility: When opening links in new tabs, consider adding aria-label or visually hidden text to inform screen reader users (e.g., "opens in new tab")

Summary

The feature implementation is solid, but the security vulnerability with target="_blank" should be fixed before merging. Once the rel attribute is added, this will be a valuable enhancement to the dropdown menu component.

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

This pull request extends link-opening behavior across the PdsLink and PdsDropdownMenuItem components. PdsLink gains a new optional target prop supporting '_blank', '_self', '_parent', and '_top' values, with the external prop now deprecated in favor of target for explicit control. PdsDropdownMenuItem similarly receives external and target props, allowing dropdown menu items to configure link behavior when an href is present. The target prop takes precedence over external when both are set. Type definitions, documentation, unit tests, and component readmes were updated accordingly to reflect these additions.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title partially relates to the changeset; it mentions the external prop addition to dropdown menu but omits the equally important target prop and pds-link modifications.
Description check ✅ Passed The description covers the problem statement, solution, components modified, and includes testing approach, but several mandatory checklist items remain unchecked and test configuration is incomplete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@libs/core/src/components/pds-dropdown-menu/pds-dropdown-menu-item/readme.md`:
- Around line 13-14: Update the README table rows for the `destructive` and
`disabled` props to tighten wording by replacing "whether or not the
dropdown-item is destructive/disabled" with "whether the dropdown-item is
destructive/disabled"; edit the descriptions for the `destructive` and
`disabled` entries accordingly to use "whether" instead of "whether or not".

In `@libs/core/src/components/pds-link/pds-link.tsx`:
- Around line 88-104: In render(), when computing targetValue (const targetValue
= this.target || (this.external ? '_blank' : undefined)) ensure you also add
rel="noopener noreferrer" to the anchor only when targetValue === '_blank' to
prevent tabnabbing; update the <a> element rendered by the pds-link component to
conditionally include a rel attribute (e.g., compute a relValue and pass it as
rel={relValue}) while keeping existing classNames(), href, id, part, target and
style usage and preserving showExternalIcon logic for the <pds-icon>.

@QuintonJason QuintonJason marked this pull request as ready for review January 28, 2026 17:43
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

A couple of small nitpicks, otherwise nice work!


### Target

The `target` prop specifies where to open the linked document. This provides more control than the `external` prop and supports all standard target values (`_blank`, `_self`, `_parent`, `_top`). When set to `_blank`, it automatically shows the external icon.
Copy link
Member

Choose a reason for hiding this comment

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

Should we always assume a new tab means it's external? I think external should handle the icon.

@QuintonJason QuintonJason merged commit 0fb8246 into main Jan 29, 2026
15 checks passed
@QuintonJason QuintonJason deleted the fix/dropdow-target-blank branch January 29, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working package: core Changes have been made to the Core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants