DSD-1613: Pattern for string or JSX element values for component title/headingText props #1453

EdwinGuzman · 2023-10-30T20:42:38Z

Fixes JIRA ticket DSD-1613

This PR does the following:

Updates the following components to accept JSX.Element type values into their "title" prop: List, NewsletterSignup,
Updates the following components to accept JSX.Element type values into their "headingText" prop: AlphabetFilter, AudioPlayer, ComponentWrapper, SearchBar, VideoPlayer
Updates the Notification component to accept JSX.Element type values into its "notificationHeading" prop.
Updates the StructuredContent component to accept JSX.Element type values into its "headingText" and "calloutText" props.
Adds a new useDSHeading React hook to make the updates above easier to implement.

How has this been tested?

Locally in Storybook.

Accessibility concerns or updates

Should help potential issues where the default, hardcoded h2 could clash in an app's heading hierarchy.

Checklist:

I have updated the Storybook documentation accordingly.
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

Front End Review:

Review the Vercel preview deployment once it is ready.

vercel · 2023-10-30T20:42:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nypl-design-system	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 27, 2023 10:14pm

src/hooks/useDSHeading.tsx

…to allow JSX.Element types

bigfishdesign13

Overall, this looks great, but there are a few things that need to be cleaned up. Please see my comments.

src/components/NewsletterSignup/NewsletterSignup.mdx

src/components/StructuredContent/StructuredContent.mdx

src/components/List/List.tsx

src/components/NewsletterSignup/NewsletterSignup.mdx

src/components/Notification/Notification.mdx

bigfishdesign13

Looks good!

EdwinGuzman · 2023-11-14T21:09:25Z

@oliviawongnyc @jackiequach can you please review this when you get a chance.

jackiequach · 2023-11-14T21:32:56Z

src/components/AlphabetFilter/AlphabetFilter.tsx

@@ -21,7 +21,7 @@ export interface AlphabetFilterProps {
  /** Value used to set the text for a `Text` component that will be displayed above the letter buttons. */
  descriptionText?: string | JSX.Element;
  /** Value used to set the text for a `Heading` component. */
-  headingText?: string;
+  headingText?: string | JSX.Element;


Should a warning be logged when a non-Heading component is passed?

jackiequach · 2023-11-14T21:33:13Z

src/components/AlphabetFilter/AlphabetFilter.tsx

@@ -21,7 +21,7 @@ export interface AlphabetFilterProps {
  /** Value used to set the text for a `Text` component that will be displayed above the letter buttons. */
  descriptionText?: string | JSX.Element;
  /** Value used to set the text for a `Heading` component. */


Update docstring to reflect changes (similarly for the other components).

jackiequach · 2023-11-14T21:41:34Z

src/components/List/List.mdx

@@ -71,6 +74,30 @@ list renders `dt` and `dd` elements.

 <Canvas of={ListStories.DescriptionList} />

+## Description List with Custom Headings


Add link to TOC.

Making this a level 3 subheading so not adding it to TOC and removing the other one that was a level 2.

…TML heading elements are passed

EdwinGuzman · 2023-11-16T19:31:16Z

@jackiequach good points. I updated the comment docstring.

EdwinGuzman · 2023-11-16T19:32:41Z

src/hooks/useDSHeading.tsx

+        // If the user passed in an HTML heading element, just use that.
+        // They will not get any DS styling.
+        updatedTitle = title;
+        console.warn(


@jackiequach @bigfishdesign13 I am logging two warnings:
1 - one where an HTML heading element is passed,
2 - one for non-DS Heading or HTML heading elements are passed.

The wording could be better. What do you think? Also added tests for these.

These are very relevant warning messages. I just made some suggetions for the wording.

jackiequach

Looks good to me!

bigfishdesign13 · 2023-11-21T23:29:02Z

src/hooks/useDSHeading.tsx

+        // They will not get any DS styling.
+        updatedTitle = title;
+        console.warn(
+          "NYPL Reservoir useDSHeading: An HTML heading element was passed " +


I think the message could use a little clarification.

An HTML heading element was passed for the `title` or `headingText` prop in a Reservoir component. The heading will render without DS-specific styling.

bigfishdesign13 · 2023-11-21T23:34:37Z

src/hooks/useDSHeading.tsx

+        );
+      } else {
+        console.warn(
+          "NYPL Reservoir useDSHeading: A DS `Heading` component or an HTML " +


Similarly...

NYPL Reservoir useDSHeading: An unsupported heading element was passed to the `title` or `headingText` prop in a Reservoir component, so that component's title will not be rendered. Instead, a DS `Heading` component or an HTML heading element should be passed.

EdwinGuzman added 2 commits October 30, 2023 09:18

init update

46a5ee4

WIP example of default/custom title prop values

e953316

EdwinGuzman commented Oct 30, 2023

View reviewed changes

src/hooks/useDSHeading.tsx Show resolved Hide resolved

EdwinGuzman added the In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. label Oct 30, 2023

EdwinGuzman requested a review from bigfishdesign13 October 30, 2023 20:44

vercel bot deployed to Preview October 30, 2023 20:45 View deployment

forgot to add updated NLS doc text

cee61b1

vercel bot deployed to Preview October 30, 2023 20:48 View deployment

bigfishdesign13 reviewed Oct 31, 2023

View reviewed changes

src/hooks/useDSHeading.tsx Show resolved Hide resolved

bigfishdesign13 reviewed Oct 31, 2023

View reviewed changes

src/hooks/useDSHeading.tsx Outdated Show resolved Hide resolved

Updating structuredcontent and notification

a6a9fa7

vercel bot deployed to Preview November 3, 2023 20:42 View deployment

Updating the rest of components that use a title or headingText prop …

8207d30

…to allow JSX.Element types

EdwinGuzman changed the title ~~WIP: Dsd 1613/pattern for title~~ DSD-1613: Pattern for string or JSX element values for component title/headingText props Nov 7, 2023

vercel bot had a problem deploying to Preview November 7, 2023 20:27 Failure

EdwinGuzman added Needs Review Pull requests that are ready for peer review. and removed In Progress Tickets or PRs that are being worked on. PRs marked as In Progress should not be merged. labels Nov 7, 2023

EdwinGuzman requested review from jackiequach, oliviawongnyc and bigfishdesign13 November 7, 2023 20:28

Merge from development and fixing conflicts

91d0045

vercel bot had a problem deploying to Preview November 7, 2023 21:24 Failure

empty

e58fd31

vercel bot had a problem deploying to Preview November 7, 2023 22:00 Failure

ugh

6d8c120

vercel bot deployed to Preview November 7, 2023 22:07 View deployment

bigfishdesign13 requested changes Nov 8, 2023

View reviewed changes

bigfishdesign13 added the Changes Requested Pull requests where changes have been requested in peer review. label Nov 8, 2023

Merging and fixing conflicts from development

5c7173e

Updating based on feedback

158fe85

vercel bot deployed to Preview November 9, 2023 22:21 View deployment

package-lock?

d8cdfbb

vercel bot deployed to Preview November 9, 2023 22:25 View deployment

EdwinGuzman removed the Changes Requested Pull requests where changes have been requested in peer review. label Nov 9, 2023

EdwinGuzman requested a review from bigfishdesign13 November 9, 2023 22:48

bigfishdesign13 approved these changes Nov 9, 2023

View reviewed changes

bigfishdesign13 added Ship It Pull requests that have been reviewed and approved. and removed Needs Review Pull requests that are ready for peer review. labels Nov 9, 2023

jackiequach reviewed Nov 14, 2023

View reviewed changes

EdwinGuzman added 2 commits November 16, 2023 13:46

Merging and fixing conflicts from development

fb1a639

Updates based on feedback and logging warnings if non-DS Heading or H…

102c352

…TML heading elements are passed

EdwinGuzman commented Nov 16, 2023

View reviewed changes

vercel bot deployed to Preview November 16, 2023 19:33 View deployment

jackiequach approved these changes Nov 17, 2023

View reviewed changes

bigfishdesign13 reviewed Nov 21, 2023

View reviewed changes

Updating warning text

e223764

vercel bot had a problem deploying to Preview November 27, 2023 21:55 Failure

EdwinGuzman added 2 commits November 27, 2023 17:07

Merging and fixing conflicts

4852e81

package-lock

6235220

vercel bot deployed to Preview November 27, 2023 22:14 View deployment

EdwinGuzman merged commit 639b867 into development Nov 27, 2023
4 checks passed

EdwinGuzman deleted the DSD-1613/pattern-for-title branch January 31, 2024 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSD-1613: Pattern for string or JSX element values for component title/headingText props #1453

DSD-1613: Pattern for string or JSX element values for component title/headingText props #1453

EdwinGuzman commented Oct 30, 2023 •

edited

Loading

vercel bot commented Oct 30, 2023 •

edited

Loading

bigfishdesign13 left a comment

bigfishdesign13 left a comment

EdwinGuzman commented Nov 14, 2023

jackiequach Nov 14, 2023

EdwinGuzman Nov 16, 2023

jackiequach Nov 14, 2023

jackiequach Nov 14, 2023

EdwinGuzman Nov 16, 2023

EdwinGuzman commented Nov 16, 2023

EdwinGuzman Nov 16, 2023

bigfishdesign13 Nov 21, 2023

jackiequach left a comment

bigfishdesign13 Nov 21, 2023

bigfishdesign13 Nov 21, 2023

		@@ -71,6 +74,30 @@ list renders `dt` and `dd` elements.

		<Canvas of={ListStories.DescriptionList} />

		## Description List with Custom Headings

DSD-1613: Pattern for string or JSX element values for component title/headingText props #1453

DSD-1613: Pattern for string or JSX element values for component title/headingText props #1453

Conversation

EdwinGuzman commented Oct 30, 2023 • edited Loading

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Checklist:

Front End Review:

vercel bot commented Oct 30, 2023 • edited Loading

bigfishdesign13 left a comment

Choose a reason for hiding this comment

bigfishdesign13 left a comment

Choose a reason for hiding this comment

EdwinGuzman commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackiequach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman commented Oct 30, 2023 •

edited

Loading

vercel bot commented Oct 30, 2023 •

edited

Loading