-
Notifications
You must be signed in to change notification settings - Fork 34
docs: add JSDoc prop descriptions to Svelte components #3327
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
base: dev
Are you sure you want to change the base?
Conversation
Add documentation comments to component props across 67 components. These descriptions enable better IDE hints and support automated documentation generation.
a1b95c1 to
8e7d486
Compare
libs/web-components/src/components/button-group/ButtonGroup.svelte
Outdated
Show resolved
Hide resolved
- Remove @required tags from props without validateRequired() calls - Fix overstated "form submission" claims to "change events" - Remove incorrect "Auto-generated" claim from Accordion id prop - Fix "Reset skeleton shapes" to "Sets the skeleton shape" Addresses feedback from initial review
| } from "../../types/relay-types"; | ||
| /** Name of the input value that is received in the _change event */ | ||
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.
remove the space
| import { quartOut } from "svelte/easing"; | ||
| // required | ||
| /** Sets the size of the spinner. */ |
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 should mention that it 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.
Added @required
| import { injectCss, type Spacing } from "../../common/styling"; | ||
| /** Horizontal spacing */ | ||
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.
empty line
| type SkeletonSize = (typeof Sizes)[number]; | ||
| /** Set component maximum width. Currently only used in card skeleton type */ | ||
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.
blank line
| /** Sets the type of the input field. */ | ||
| export let type: Type = "text"; | ||
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.
maybe remove all the spaces in between props to keep things consistent
| type Variant = (typeof Variants)[number]; | ||
| // 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.
no longer needed
| import type { Spacing } from "../../common/styling"; | ||
| /** Gap between child items */ | ||
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.
space
| /** Accepted file types as a comma-separated list of MIME types or file extensions (e.g., "image/*,.pdf"). */ | ||
| export let accept: string = "*"; | ||
| /** Maximum file size with unit (e.g., "5MB", "100KB", "1GB"). Files exceeding this will be rejected. */ | ||
| export let maxfilesize: string = "5MB"; |
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.
It would be nice if the default value was contained in the comment.
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.
added
| import type { Spacing } from "../../common/styling"; | ||
| /** @required The title heading */ | ||
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.
blank line
libs/web-components/src/components/filter-chip/FilterChip.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/hero-banner/HeroBanner.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/icon-button/IconButton.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/microsite-header/MicrositeHeader.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/microsite-header/MicrositeHeader.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/notification/Notification.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/pagination/Pagination.svelte
Outdated
Show resolved
Hide resolved
| export let disabled: string = "false"; | ||
| /** Shows an error state on all radio items in the group. */ | ||
| export let error: string = "false"; | ||
| /** The design system version for styling purposes. */ |
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.
remove comment for version
libs/web-components/src/components/side-menu-group/SideMenuGroup.svelte
Outdated
Show resolved
Hide resolved
libs/web-components/src/components/side-menu-heading/SideMenuHeading.svelte
Outdated
Show resolved
Hide resolved
vanessatran-ddi
left a comment
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.
I left all the comments.
- Fix formatting: remove blank lines between JSDoc and exports - Standardize testid descriptions across components - Add @internal tag to version props (not for public docs) - Add @internal to multiselect in Dropdown (not exposed) - Add @required to Spinner size (no default value) - Fix inaccurate descriptions (Callout size, Dropdown icon/maxheight) - Add missing docs (DatePicker size, Button/IconButton/LinkButton actions) - Add default value to FileUploadInput maxfilesize description - Remove redundant // required comment in IconButton
|
@chrisolsen @vanessatran-ddi What changed:
One note: I kept MicrositeHeader's version as a public prop. That's the app version shown in the header, not the design system version. Regarding framework-specific props (as/tag, ReactNode/TemplateRef): For now, the comments in Svelte can at least stay as the source of truth for descriptions. We'll sort out how to show Angular vs React differences on the website as well, probably based on the framework selector. I will create a new PR for that. |
vanessatran-ddi
left a comment
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.
The rest looks good! There are some files missing JS doc
| export let size: BadgeSize = "medium"; | ||
| /** Sets the visual emphasis. 'subtle' for less prominent, 'strong' for more emphasis. */ | ||
| export let emphasis: (typeof emphasisLevels)[number] = "strong"; | ||
| /** The design system version for styling purposes. */ |
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.
missing set internal
| /** Sets a custom width for the button (e.g., "200px" or "100%"). */ | ||
| export let width: string = ""; | ||
| /** Design system version. Version 2 includes updated styling and accessibility improvements. */ |
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.
Missing internal on the comment
| handleKeyDown: (e: KeyboardEvent) => void; | ||
| } | ||
| // Props |
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.
We are missing document for DropdownItem.svelte
| /** Sets the size of the input. 'compact' reduces height for dense layouts. */ | ||
| export let size: "default" | "compact" = "default"; | ||
| /** The design system version for styling purposes. */ |
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.
missing @internal
| import { GoAIconType } from "../icon/Icon.svelte"; | ||
| import { dispatch, receive, relay } from "../../common/utils"; | ||
| import { MenuAction } from "./MenuAction.svelte"; | ||
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.
Missing document for MenuAction.svelte
| // Public | ||
| /** Width of the table. By default it will fit the enclosed content. */ |
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.
We are missing document for TableSortHeader.svelte
| export let initialtab: number = -1; | ||
| /** Sets a data-testid attribute for automated testing. */ | ||
| export let testid: string = ""; | ||
| /** The design system version for styling purposes. */ |
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.
Missing @internal
| | "progress"; | ||
| // Props | ||
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.
We are missing TemporaryNotificationCtrl.svelte
Summary
Changes
Example
Testing
Created with the help of claude code