Feature:SPLaSK compliance on link and button component(SSD-168) #320
Feature:SPLaSK compliance on link and button component(SSD-168) #3204fn4n wants to merge 4 commits into
Conversation
…vices, Freedom of Information, Online Procurement Announcement, E-participation, privacy policy on link component and E-participation on button component
🦋 Changeset detectedLatest commit: 26c54c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds SPLaSK (Social Platform for Learning and Sharing Knowledge) compliance support to the Link and Button components by introducing wrapper attributes that conditionally wrap components with custom HTML elements.
- Added JSX type declarations for SPLaSK custom elements in the Link component
- Implemented conditional wrapper logic for both Link and Button components based on SPLaSK attributes
- Added support for 7 SPLaSK variants in Link component and 1 in Button component
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/components/link.tsx | Added JSX declarations for SPLaSK elements and conditional wrapper logic with 7 SPLaSK attribute variants |
| packages/react/src/components/button.tsx | Added SplaskOnlineEParticipation attribute with conditional wrapper for e-participation compliance |
| .changeset/lemon-tables-beam.md | Added changeset entry documenting the SPLaSK compliance feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const buttonElement = ( | ||
| <Comp | ||
| ref={ref} | ||
| className={clx(button_cva({ variant, size, className, iconOnly }))} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
The ButtonContext.Provider wrapper has been removed, which will break components that depend on the button context (like ButtonIcon or ButtonText). The context should be preserved around the button element.
| <div splwpk-online-e-participation="splwpk-online-e-participation"> | ||
| {buttonElement} | ||
| </div> |
There was a problem hiding this comment.
The SPLaSK attribute should use the JSX type declaration pattern from the Link component. Add the custom element declaration and use the proper JSX element instead of a div with custom attributes.
| <div splwpk-online-e-participation="splwpk-online-e-participation"> | |
| {buttonElement} | |
| </div> | |
| <splask-online-e-participation> | |
| {buttonElement} | |
| </splask-online-e-participation> |
| * @example | ||
| * <Link href="https://design.digital.gov.my" newTab primary underline="always">MYDS</Link> | ||
| * @see {@link https://design.digital.gov.my/?path=/docs/myds-react-link--docs} | ||
| * <Link href="https://design.digital.gov.my" newTab primary underline="always" SplaskBroadcast> |
There was a problem hiding this comment.
The example documentation shows SplaskBroadcast as a boolean prop, but it should include the proper value assignment (SplaskBroadcast={true}) to demonstrate correct usage.
| * <Link href="https://design.digital.gov.my" newTab primary underline="always" SplaskBroadcast> | |
| * <Link href="https://design.digital.gov.my" newTab primary underline="always" SplaskBroadcast={true}> |
|
Updated @harrisazmi |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // collect Splask attributes | ||
| const attrs: Record<string, string> = {}; | ||
| if (SplaskBroadcast) attrs["splwpk-broadcast"] = "splwpk-broadcast"; | ||
| if (SplaskOnlineServices) attrs["splwpk-online-services"] = "splwpk-online-services"; | ||
| if (SplaskOnlineEParticipation) attrs["splwpk-online-e-participation"] = "splwpk-online-e-participation"; | ||
| if (SplaskPrivacyPolicy) attrs["splwpk-privacy-policy"] = "splwpk-privacy-policy"; | ||
| if (SplaskProcurement) attrs["splwpk-procurement"] = "splwpk-procurement"; | ||
| if (SplaskFreedom) attrs["splwpk-freedom"] = "splwpk-freedom"; | ||
| if (SplaskFaqs) attrs["splwpk-faqs"] = "splwpk-faqs"; | ||
|
|
||
| if (Object.keys(attrs).length === 0) return linkElement; | ||
|
|
||
| return <div {...attrs}>{linkElement}</div>; |
There was a problem hiding this comment.
Spreading Record<string, string> into a JSX element typically fails TypeScript checks because it implies every possible prop name (e.g. onClick) is a string, which is incompatible with React’s event handler prop types. Also, the keys like "splwpk-broadcast" are being used as custom attributes on a div, but the earlier JSX.IntrinsicElements augmentation declares them as custom elements (tags), not attributes, so it doesn’t address the typing issue. Consider rendering actual wrapper custom elements (e.g. <splwpk-broadcast>...</splwpk-broadcast>) or switching to data-* attributes (which are supported/typable), and avoid using a generic Record<string,string> spread as JSX props.
| // collect Splask attributes | |
| const attrs: Record<string, string> = {}; | |
| if (SplaskBroadcast) attrs["splwpk-broadcast"] = "splwpk-broadcast"; | |
| if (SplaskOnlineServices) attrs["splwpk-online-services"] = "splwpk-online-services"; | |
| if (SplaskOnlineEParticipation) attrs["splwpk-online-e-participation"] = "splwpk-online-e-participation"; | |
| if (SplaskPrivacyPolicy) attrs["splwpk-privacy-policy"] = "splwpk-privacy-policy"; | |
| if (SplaskProcurement) attrs["splwpk-procurement"] = "splwpk-procurement"; | |
| if (SplaskFreedom) attrs["splwpk-freedom"] = "splwpk-freedom"; | |
| if (SplaskFaqs) attrs["splwpk-faqs"] = "splwpk-faqs"; | |
| if (Object.keys(attrs).length === 0) return linkElement; | |
| return <div {...attrs}>{linkElement}</div>; | |
| const hasSplaskAttrs = | |
| SplaskBroadcast || | |
| SplaskOnlineServices || | |
| SplaskOnlineEParticipation || | |
| SplaskPrivacyPolicy || | |
| SplaskProcurement || | |
| SplaskFreedom || | |
| SplaskFaqs; | |
| if (!hasSplaskAttrs) return linkElement; | |
| return ( | |
| <div | |
| splwpk-broadcast={ | |
| SplaskBroadcast ? "splwpk-broadcast" : undefined | |
| } | |
| splwpk-online-services={ | |
| SplaskOnlineServices ? "splwpk-online-services" : undefined | |
| } | |
| splwpk-online-e-participation={ | |
| SplaskOnlineEParticipation | |
| ? "splwpk-online-e-participation" | |
| : undefined | |
| } | |
| splwpk-privacy-policy={ | |
| SplaskPrivacyPolicy ? "splwpk-privacy-policy" : undefined | |
| } | |
| splwpk-procurement={ | |
| SplaskProcurement ? "splwpk-procurement" : undefined | |
| } | |
| splwpk-freedom={SplaskFreedom ? "splwpk-freedom" : undefined} | |
| splwpk-faqs={SplaskFaqs ? "splwpk-faqs" : undefined} | |
| > | |
| {linkElement} | |
| </div> | |
| ); |
|
|
||
| if (Object.keys(attrs).length === 0) return linkElement; | ||
|
|
||
| return <div {...attrs}>{linkElement}</div>; |
There was a problem hiding this comment.
Wrapping Link in a div changes the component’s default inline behavior and can produce invalid HTML in common cases (e.g., a link used inside text/paragraph flow). If a wrapper is required, prefer an inline wrapper (span) or the actual SPLaSK custom wrapper element so layout/semantics remain consistent.
| return <div {...attrs}>{linkElement}</div>; | |
| return <span {...attrs}>{linkElement}</span>; |
| declare global { | ||
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| "splwpk-broadcast": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-online-services": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-online-e-participation": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-privacy-policy": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-procurement": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-freedom": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| "splwpk-faqs": React.DetailedHTMLProps< | ||
| React.HTMLAttributes<HTMLElement>, | ||
| HTMLElement | ||
| >; | ||
| } |
There was a problem hiding this comment.
The declare global JSX augmentation is embedded in the component file, which makes the component responsible for global type setup and can lead to duplication/ordering issues as custom tags grow. Prefer moving this to a dedicated *.d.ts (e.g. splask-elements.d.ts) in the React package so it’s clearly global typing, easier to maintain, and doesn’t mix type plumbing with component logic.
| declare global { | |
| namespace JSX { | |
| interface IntrinsicElements { | |
| "splwpk-broadcast": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-online-services": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-online-e-participation": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-privacy-policy": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-procurement": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-freedom": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-faqs": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| } | |
| declare namespace JSX { | |
| interface IntrinsicElements { | |
| "splwpk-broadcast": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-online-services": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-online-e-participation": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-privacy-policy": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-procurement": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-freedom": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; | |
| "splwpk-faqs": React.DetailedHTMLProps< | |
| React.HTMLAttributes<HTMLElement>, | |
| HTMLElement | |
| >; |
| // Splask attributes | ||
| SplaskBroadcast?: boolean; | ||
| SplaskOnlineServices?: boolean; | ||
| SplaskOnlineEParticipation?: boolean; | ||
| SplaskPrivacyPolicy?: boolean; | ||
| SplaskProcurement?: boolean; | ||
| SplaskFreedom?: boolean; | ||
| SplaskFaqs?: boolean; |
There was a problem hiding this comment.
The new prop names are PascalCase (SplaskBroadcast, etc.), which is atypical for React props and can read like components. Consider renaming to camelCase (e.g. splaskBroadcast, splaskOnlineServices, …) for consistency with common React conventions.
| const ButtonContext = createContext<VariantProps<typeof button_cva>>({ | ||
| variant: "primary-fill", | ||
| size: "small", | ||
| iconOnly: false, | ||
| }); |
There was a problem hiding this comment.
ButtonContext is typed as VariantProps<typeof button_cva>, but the default value and provider value include iconOnly. Since iconOnly is declared on ButtonProps separately (not as a button_cva variant in this diff), this is very likely a TypeScript type error. Fix by changing the context type to include iconOnly explicitly (e.g. a dedicated interface/type that includes variant, size, and iconOnly) or by making iconOnly an actual button_cva variant if that’s the intent.
| return ( | ||
| <ButtonContext.Provider value={{ variant, size }}> | ||
| const buttonElement = ( | ||
| <ButtonContext.Provider value={{ variant, size, iconOnly }}> |
There was a problem hiding this comment.
ButtonContext is typed as VariantProps<typeof button_cva>, but the default value and provider value include iconOnly. Since iconOnly is declared on ButtonProps separately (not as a button_cva variant in this diff), this is very likely a TypeScript type error. Fix by changing the context type to include iconOnly explicitly (e.g. a dedicated interface/type that includes variant, size, and iconOnly) or by making iconOnly an actual button_cva variant if that’s the intent.
| if (!SplaskOnlineEParticipation) return buttonElement; | ||
|
|
||
| return ( | ||
| <div splwpk-online-e-participation="splwpk-online-e-participation"> |
There was a problem hiding this comment.
splwpk-online-e-participation is a non-standard DOM attribute; React may pass it through at runtime, but TypeScript’s JSX typings typically reject it on a div. Align this with the approach used for custom elements by wrapping with a real custom element tag (e.g. <splwpk-online-e-participation>...</splwpk-online-e-participation>) or use a data-* attribute (e.g. data-splwpk-online-e-participation) to keep it valid/typable.
| <div splwpk-online-e-participation="splwpk-online-e-participation"> | |
| <div data-splwpk-online-e-participation="splwpk-online-e-participation"> |
Summarise the feature
Issue ticket # 168, 194, 195, 196
Description:
SPLaSK compliance - Broadcast, Number of online services, Freedom of Information, Online Procurement Announcement, E-participation, privacy policy on link component and E-participation on button component
Type of change
Affected endpoints
none
Checklist