code review feedback

- test the DOM element exists in RenderElement tests
- move the CustomLinkComponent out of the example, to avoid folks copying it
- render element docs page broken by linter, props merging section now appears

.changeset/tender-avocados-develop.md

@@ -2,6 +2,6 @@
 "@salt-ds/core": minor
 ---
 
-add `render` prop to `NavigationItem`
+Added `render` prop to `NavigationItem`
 
 - the `render` prop enables the substitution of the default anchor tag with an alternate link, such as React Router, facilitating integration with routing libraries.

packages/core/src/__tests__/__e2e__/navigation-item/NavigationItem.cy.tsx

@@ -1,4 +1,4 @@
-import { NavigationItem, NavigationItemRenderProps } from "@salt-ds/core";
+import { NavigationItem } from "@salt-ds/core";
 import { NotificationIcon } from "@salt-ds/icons";
 
 describe("GIVEN a NavItem", () => {
@@ -103,7 +103,10 @@ describe("GIVEN a NavItem", () => {
 
   describe("AND `render` is passed a render function", () => {
     it("should call `render` to create parent item", () => {
-      const mockRender = cy.stub().as("render");
+      const mockRender = cy
+        .stub()
+        .as("render")
+        .returns(<button>Parent Button</button>);
       cy.mount(
         <NavigationItem
           active={true}
@@ -117,6 +120,7 @@ describe("GIVEN a NavItem", () => {
           Navigation Item
         </NavigationItem>
       );
+      cy.findByText("Parent Button").should("exist");
       cy.get("@render").should("have.been.calledWithMatch", {
         parent: true,
         active: true,
@@ -133,7 +137,10 @@ describe("GIVEN a NavItem", () => {
       });
     });
     it("should call `render` to create child item", () => {
-      const mockRender = cy.stub().as("render");
+      const mockRender = cy
+        .stub()
+        .as("render")
+        .returns(<a>Navigation Link</a>);
       cy.mount(
         <NavigationItem
           active={true}
@@ -147,6 +154,7 @@ describe("GIVEN a NavItem", () => {
           Navigation Item
         </NavigationItem>
       );
+      cy.findByText("Navigation Link").should("exist");
       cy.get("@render").should("have.been.calledWithMatch", {
         parent: false,
         active: true,
@@ -168,10 +176,7 @@ describe("GIVEN a NavItem", () => {
   describe("AND `render` is given a JSX element", () => {
     it("should merge the props and render the JSX element ", () => {
       cy.mount(
-        <NavigationItem
-          parent={true}
-          render={<button id={"button"}>Button Children</button>}
-        >
+        <NavigationItem parent={true} render={<button>Button Children</button>}>
           Navigation Item
         </NavigationItem>
       );

packages/core/src/navigation-item/ExpansionIcon.tsx

@@ -12,7 +12,7 @@ const iconExpansionMap = {
   },
 };
 
-type ExpansionIconProps = {
+interface ExpansionIconProps {
   /**
    * Whether the navigation item is expanded.
    */
@@ -25,7 +25,7 @@ type ExpansionIconProps = {
    * The orientation of the navigation item.
    */
   orientation?: "horizontal" | "vertical";
-};
+}
 
 export const ExpansionIcon: FC<ExpansionIconProps> = ({
   expanded = false,

packages/core/src/navigation-item/NavigationItem.tsx

@@ -27,7 +27,7 @@ export interface NavigationItemRenderProps
     "active" | "expanded" | "level" | "parent" | "orientation"
   > {
   /**
-   * Props to apply to the chld row to render a link
+   * Props to apply to the child row to render a link
    */
   linkProps?: AnchorHTMLAttributes<HTMLAnchorElement>;
   /**
@@ -128,7 +128,7 @@ export const NavigationItem = forwardRef<HTMLDivElement, NavigationItemProps>(
       "--saltNavigationItem-level": `${level}`,
     };
 
-    const isParent = parent || href === undefined; // for backwards compatiblity with original
+    const isParent = parent || href === undefined;
     const elementProps = {
       className: clsx(
         withBaseName("wrapper"),

packages/core/stories/navigation-item/navigation-item.stories.tsx

@@ -589,13 +589,14 @@ export const VerticalNestedGroupNoIcon = () => {
   );
 };
 
+const CustomLinkImplementation = (props: NavigationItemRenderProps) => (
+  <a {...props} aria-label={"overridden-label"}>
+    <Text>Your Own Link Implementation</Text>
+  </a>
+);
+
 export const WithRenderElement = () => {
   const [expanded, setExpanded] = useState<boolean>(false);
-  const CustomLinkImplementation = (props: NavigationItemRenderProps) => (
-    <a {...props} aria-label={"overridden-label"}>
-      <Text>Your Own Link Implementation</Text>
-    </a>
-  );
   return (
     <nav>
       <StackLayout

site/docs/components/navigation-item/examples.mdx

@@ -96,37 +96,35 @@ When the user collapses a vertical nested group, and there is an active nested i
   <LivePreview componentName="navigation-item" exampleName="RenderElement">
 ## Render prop element
 
-Using the `render` prop element, you can customise the tag rendered by your `NavigationItem`.
+Using the `render` prop, you can customize the element rendered by the `NavigationItem`.
 Props defined on the JSX element will be merged with props from the `NavigationItem`.
 
-A `NavigationItem` will either act as a navigational link, or an expandable group for other `NavigationItem` elements.
+A `NavigationItem` will either act as a link, or an expandable group for other `NavigationItem` elements.
 When a parent group is clicked, the group will expand or collapse.
 When a link is clicked, navigates the user to the specified `href` URL.
 
-The default element for parent groups is a HTML button tag, whilst a link will use a HTML anchor tag.
+The default element for parent groups is a `<button>`, whilst a link will use a `<a>`.
 
 ### Prop merging
 
-`<NavigationItem href="/your-page" render={<a target="_blank"/>} />`
+```
+<NavigationItem href="/your-page" render={<a target="_blank"/>} />
+```
 
-<br />
-<br />
-renders
-<br />
-<br />`
-<a href="/your-page" target="_blank">
-  ...
-</a>
-`
+will be created in the DOM as an `<a/>` with merged props
+
+```
+<a href="/your-page" target="_blank">...</a>
+```
 
 </LivePreview>
 
   <LivePreview componentName="navigation-item" exampleName="RenderProp">
 ## Render prop callback
 
-Using the `render` prop element, you can customise the tag rendered by your `NavigationItem`.
+Using the `render` prop, you can customize the element rendered by the `NavigationItem`.
 
-The render prop can also accept a function. This approach grants enhanced control over how props are merged, allowing for more precise customization of component behavior.
+The render prop can also accept a function. This approach allows more control over how props are merged, allowing for more precise customization of the component's behavior.
 
 When a function is passed to the render prop, it's your responsibility to merge props within the function itself.
 

site/src/examples/navigation-item/RenderElement.tsx

@@ -6,13 +6,14 @@ import {
   Text,
 } from "@salt-ds/core";
 
+const CustomLinkImplementation = (props: NavigationItemRenderProps) => (
+  <a {...props} aria-label={"overridden-label"}>
+    <Text>Your Own Link Implementation</Text>
+  </a>
+);
+
 export const RenderElement = (): ReactElement => {
   const [expanded, setExpanded] = useState<boolean>(false);
-  const CustomLinkImplementation = (props: NavigationItemRenderProps) => (
-    <a {...props} aria-label={"overridden-label"}>
-      <Text>Your Own Link Implementation</Text>
-    </a>
-  );
   return (
     <nav>
       <StackLayout