From c3c8ad5c1ff5566454e50d66481fad4066a64d4d Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 12 Apr 2023 12:18:54 -0500 Subject: [PATCH 1/3] docs(adr): add internal modules ADR (#3046) * docs(adr): add internal modules ADR * Update and rename adr-014-internal-modules.md to adr-016-internal-modules.md --- .../adrs/adr-016-internal-modules.md | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 contributor-docs/adrs/adr-016-internal-modules.md diff --git a/contributor-docs/adrs/adr-016-internal-modules.md b/contributor-docs/adrs/adr-016-internal-modules.md new file mode 100644 index 00000000000..cc91c68e3f3 --- /dev/null +++ b/contributor-docs/adrs/adr-016-internal-modules.md @@ -0,0 +1,65 @@ +# ADR 016: Internal Modules + +## Status + +| Stage | Status | +| -------- | ------ | +| Approved | ✅ | +| Adopted | 🚧 | + +## Context + +Currently all files live under the `src` directory. In the `npm` package for `@primer/react`, we specify the following export pattern: + +```json5 +{ + "exports": { + // ... + "./lib-esm/*": { + "import": [ + // ... + ], + "require": [ + // ... + ] + } + } +} +``` + +This pattern, along with our Rollup setup, opts-in files and folders under the `src` directory into the public API of the package. This means that certain parts of the codebase which aren't intended to be used outside of `@primer/react` are considered part of its public API and can be imported and used by consumers. + +## Decision + +Adopt a convention in Primer React where internal modules live in the `src/internal` folder. This folder which would include all components, hooks, and other modules which are not intended for usage outside of the project. Files within `src/internal` may be grouped by area, such as `src/internal/components`, `src/internal/hooks`, etc. + +In the `"exports"` field of our `npm` package, we can then add the following pattern: + +```json5 +{ + "exports": { + // ... + "./lib-esm/internal/*": null + } +} +``` + +This pattern would remove any files and folders within `src/internal` from the public API of the `npm` package. This pattern is inspired by [this section](https://nodejs.org/api/packages.html#package-entry-points) from Node.js, specifically this example: + +```json +{ + "name": "my-package", + "exports": { + ".": "./lib/index.js", + "./feature/*.js": "./feature/*.js", + "./feature/internal/*": null + } +} +``` + + +### Impact + +- Update the `"exports"` field in `package.json` to exclude `./lib-esm/internal` from usage +- (In a major release) Move internal-only files into internal folder +- For internal modules that are being created after this ADR is accepted, add them to the `src/internal` folder From e6a5db69a11626b3e0f585fbf0816343276a7334 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 12 Apr 2023 16:31:53 -0500 Subject: [PATCH 2/3] fix(Tooltip): update TypeScript types for TooltipProps (#3159) * fix(Tooltip): update TypeScript types for TooltipProps * chore: add changeset --------- Co-authored-by: Josh Black --- .changeset/dirty-files-talk.md | 5 ++ src/Tooltip/Tooltip.tsx | 54 +++++++++++--------- src/Tooltip/__tests__/Tooltip.types.test.tsx | 4 ++ 3 files changed, 38 insertions(+), 25 deletions(-) create mode 100644 .changeset/dirty-files-talk.md diff --git a/.changeset/dirty-files-talk.md b/.changeset/dirty-files-talk.md new file mode 100644 index 00000000000..e901486371a --- /dev/null +++ b/.changeset/dirty-files-talk.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Update TooltipProps to extend from StyledTooltip props diff --git a/src/Tooltip/Tooltip.tsx b/src/Tooltip/Tooltip.tsx index 9be7dbf95f7..badfd7e7993 100644 --- a/src/Tooltip/Tooltip.tsx +++ b/src/Tooltip/Tooltip.tsx @@ -7,28 +7,9 @@ import {invariant} from '../utils/invariant' import styled from 'styled-components' import {get} from '../constants' import {useOnEscapePress} from '../hooks/useOnEscapePress' +import {ComponentProps} from '../utils/types' -export type TooltipProps = { - direction?: 'n' | 'ne' | 'e' | 'se' | 's' | 'sw' | 'w' | 'nw' - text?: string - noDelay?: boolean - align?: 'left' | 'right' - wrap?: boolean - type?: 'label' | 'description' - 'aria-label'?: React.AriaAttributes['aria-label'] -} & SxProp - -export type TriggerPropsType = { - 'aria-describedby'?: string - 'aria-labelledby'?: string - 'aria-label'?: string - onBlur?: React.FocusEventHandler - onFocus?: React.FocusEventHandler - onMouseEnter?: React.MouseEventHandler - ref?: React.RefObject -} - -const TooltipEL = styled.div` +const StyledTooltip = styled.div` // tooltip element itself position: absolute; z-index: 1000000; @@ -294,7 +275,30 @@ const TooltipEL = styled.div` ${sx}; ` -export const Tooltip: React.FC> = ({ +export type TooltipProps = React.PropsWithChildren< + { + direction?: 'n' | 'ne' | 'e' | 'se' | 's' | 'sw' | 'w' | 'nw' + text?: string + noDelay?: boolean + align?: 'left' | 'right' + wrap?: boolean + type?: 'label' | 'description' + 'aria-label'?: React.AriaAttributes['aria-label'] + } & SxProp & + ComponentProps +> + +export type TriggerPropsType = { + 'aria-describedby'?: string + 'aria-labelledby'?: string + 'aria-label'?: string + onBlur?: React.FocusEventHandler + onFocus?: React.FocusEventHandler + onMouseEnter?: React.MouseEventHandler + ref?: React.RefObject +} + +export const Tooltip = ({ direction = 'n', // used for description type text, @@ -306,7 +310,7 @@ export const Tooltip: React.FC> = ({ type = 'label', children, ...rest -}) => { +}: TooltipProps) => { const id = useId() const triggerRef = useRef(null) const child = Children.only(children) @@ -376,7 +380,7 @@ export const Tooltip: React.FC> = ({ child.props.onMouseEnter?.(event) }, })} - > = ({ id={id} > {text ?? label} - + ) } diff --git a/src/Tooltip/__tests__/Tooltip.types.test.tsx b/src/Tooltip/__tests__/Tooltip.types.test.tsx index 65d27666df2..758d05377e7 100644 --- a/src/Tooltip/__tests__/Tooltip.types.test.tsx +++ b/src/Tooltip/__tests__/Tooltip.types.test.tsx @@ -5,6 +5,10 @@ export function shouldAcceptCallWithNoProps() { return } +export function shouldAcceptAdditionalProps() { + return +} + export function shouldNotAcceptSystemProps() { // @ts-expect-error system props should not be accepted return From 36dbb73849e700010fb21f88032ebb507b95b2e2 Mon Sep 17 00:00:00 2001 From: Tyler Benning Date: Wed, 12 Apr 2023 16:22:49 -0600 Subject: [PATCH 3/3] refactor(SubNav): move files, create storybook (#3151) * refactor SubNav component similar to TabNav * Update generated/components.json * Update src/SubNav/index.ts --------- Co-authored-by: tbenning Co-authored-by: Cole Bemis --- docs/content/SubNav.mdx | 2 +- generated/components.json | 137 +++++++++++++------------ src/{ => SubNav}/SubNav.docs.json | 0 src/SubNav/SubNav.features.stories.tsx | 24 +++++ src/SubNav/SubNav.stories.tsx | 45 ++++++++ src/{ => SubNav}/SubNav.tsx | 6 +- src/SubNav/index.ts | 1 + src/__tests__/storybook.test.tsx | 1 + 8 files changed, 146 insertions(+), 70 deletions(-) rename src/{ => SubNav}/SubNav.docs.json (100%) create mode 100644 src/SubNav/SubNav.features.stories.tsx create mode 100644 src/SubNav/SubNav.stories.tsx rename src/{ => SubNav}/SubNav.tsx (96%) create mode 100644 src/SubNav/index.ts diff --git a/docs/content/SubNav.mdx b/docs/content/SubNav.mdx index 0aac44eee86..849fb8bb29d 100644 --- a/docs/content/SubNav.mdx +++ b/docs/content/SubNav.mdx @@ -4,7 +4,7 @@ title: SubNav status: Alpha --- -import data from '../../src/SubNav.docs.json' +import data from '../../src/SubNav/SubNav.docs.json' Use the SubNav component for navigation on a dashboard-type interface with another set of navigation components above it. This helps distinguish navigation hierarchy. diff --git a/generated/components.json b/generated/components.json index 32289f70c61..5f410ab0fe2 100644 --- a/generated/components.json +++ b/generated/components.json @@ -210,72 +210,6 @@ ], "subcomponents": [] }, - "sub_nav": { - "id": "sub_nav", - "name": "SubNav", - "status": "alpha", - "a11yReviewed": false, - "stories": [], - "props": [ - { - "name": "actions", - "type": "React.ReactNode", - "description": "Place another element, such as a button, to the opposite side of the navigation items." - }, - { - "name": "align", - "type": "'right'", - "description": "Use `right` to have navigation items aligned right." - }, - { - "name": "full", - "type": "boolean", - "description": "Used to make navigation fill the width of the container." - }, - { - "name": "aria-label", - "type": "string", - "description": "Used to set the `aria-label` on the top level `