Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WNMGDS-3191] Box Content Component #3419

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

jack-ryan-nava-pbc
Copy link
Collaborator

@jack-ryan-nava-pbc jack-ryan-nava-pbc commented Jan 30, 2025

Summary

  • Added initial styling (no tokens yet)
  • Added initial markup

How to test

  1. Pull down
  2. Build branch
  3. Run in Storybook
  4. Use screen reader to check content
  5. Use Axe devtools to confirm there are no a11y errors

Demo Branch

Note: This is a pull request to solicit feedback on initial approach and ways to proceed. The final pull request will address the following items in addition to feedback gathered here.

  • Description of component or pattern
  • HTML code snippet example(s)
  • React code snippet example(s)
  • All React props are explained
  • General guidance or best practices
  • Usability - when to use
  • Usability - when to consider something else
  • Accessibility considerations

Checklist

  • Prefixed the PR title with the Jira ticket number as [WNMGDS-####] 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

@jack-ryan-nava-pbc jack-ryan-nava-pbc added Impacts: Core Impacts the core DS primarily, changes may occur in other themes as well. Type: Added Indicates a new feature. labels Jan 30, 2025
@jack-ryan-nava-pbc jack-ryan-nava-pbc self-assigned this Jan 30, 2025
pwolfert and others added 8 commits January 31, 2025 17:06
…rough automated testing (#3407)

* Add a new astro project that specifically installs React 17

* Add a similar project for React 18

* Add VRTs for the two react projects so our automated testing suite will validate those versions

* Update readme

* Let's just leave this project alone for now

* Move the web components template into the new `_shared` folder
* Initial commit

* Update import path for define to use core design system module within monorepo.

* Adds onClickLinkAnalytics handling

* Adds unit tests

* Adds exclude list to tsconfig.json

* Adds an alias for relative path to define in webpack config.

* Adds an import to react in ds-simple-footer.test.tsx

* Adds react as an import to ds-accordion and ds-alert test and storybook files.

* Add snapshots

* Adds storybook-doc snapshot

* Remove exclude list in tsconfig.json

* Adds default values for argTypes

* Updates storybook-docs snap

* Revert out of scope changes.

* Revert out of scope changes, part 2

* We no longer need to export define from index.ts

* Removes setTimeout from useEffect in storybook template.

* Adds website info argtype to storybook file, updates storybook-doc snapshot
@jack-ryan-nava-pbc jack-ryan-nava-pbc marked this pull request as ready for review February 3, 2025 16:43
@tamara-corbalt
Copy link
Collaborator

Looking really good!

There's a few commits in this branch's commit history that don't look like they belong: (f4264e2 & d43477b)

@jack-ryan-nava-pbc
Copy link
Collaborator Author

Noting here that forced colors mode is now supported:

Screenshot 2025-02-04 at 10 01 08 PM Screenshot 2025-02-04 at 10 01 17 PM Screenshot 2025-02-04 at 10 01 24 PM

if (author) {
return `${author} `;
}
if (!citation && !author) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a bit heavy handed???

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think so. There are a few examples (ChoiceList) where warnings are defined like so:

  if (process.env.NODE_ENV !== 'production') {
    if (props.type !== 'checkbox' && props.choices.length === 1) {
      console.warn(
        `[Warning]: Use type="checkbox" for components with only one choice. A single radio button is disallowed because it prevents users from deselecting the field.`
      );
    }
  }

className?: string;
}

export type BoxQuotationProps = RequireAtLeastOne<MinimumBoxQuotationProps, 'citation' | 'author'>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is neat! I wonder if it's worth adding an note in the MinimumBoxQuotationProps that either author or citation is required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! Yes that is a good idea.

Copy link
Collaborator

@kim-cmsds kim-cmsds left a comment

Choose a reason for hiding this comment

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

Nice work!

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! Approve pending CI

@jack-ryan-nava-pbc jack-ryan-nava-pbc added this to the 12.1.1 milestone Feb 12, 2025
@jack-ryan-nava-pbc
Copy link
Collaborator Author

Moving this to draft until content review is complete and design is finalized.

@jack-ryan-nava-pbc jack-ryan-nava-pbc marked this pull request as draft February 18, 2025 19:32
@jack-ryan-nava-pbc jack-ryan-nava-pbc marked this pull request as ready for review February 25, 2025 16:11
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: Added Indicates a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants