-
Notifications
You must be signed in to change notification settings - Fork 88
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
base: main
Are you sure you want to change the base?
Conversation
packages/design-system/src/components/BoxContent/BoxContent.tsx
Outdated
Show resolved
Hide resolved
…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
… throw error at runtime if missing props
if (author) { | ||
return `${author} `; | ||
} | ||
if (!citation && !author) { |
There was a problem hiding this comment.
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???
There was a problem hiding this comment.
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'>; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
packages/design-system/src/components/BoxContent/BoxQuotation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
… update docs snapshot
There was a problem hiding this 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
Moving this to draft until content review is complete and design is finalized. |
Summary
How to test
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.
props
are explainedChecklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.