fix(app-degree-pages): refactor defaultProps to use JS default params #1596
Conversation
|
Storybook deployed at https://unity-uds-staging.s3.us-west-2.amazonaws.com/pr-1596/index.html |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates React components from using the deprecated defaultProps pattern to modern default parameter values in function signatures. This aligns with React 18.3+ best practices where defaultProps is being deprecated for function components.
- Removed all
Component.defaultPropsdeclarations across multiple packages - Added default parameter values directly in component function signatures
- Renamed imported
defaultPropsto avoid naming conflicts in news components
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/component-news/src/components/CardListlNews/index.jsx | Renamed defaultProps import to coreDefaultProps to avoid conflict with removed static property |
| packages/component-news/src/components/CardGridNews/index.jsx | Renamed defaultProps import to coreDefaultProps to avoid conflict with removed static property |
| packages/component-news/src/components/CardCarouselNews/index.jsx | Renamed defaultProps import to coreDefaultProps to avoid conflict with removed static property |
| packages/component-header/src/header.js | Migrated from ASUHeader.defaultProps to default parameters in function signature |
| packages/component-header-footer/src/footer/components/Social/index.js | Migrated from Social.defaultProps to default parameters with nested destructuring |
| packages/component-footer/src/components/Social/index.js | Migrated from Social.defaultProps to default parameters with nested destructuring |
| packages/component-footer/src/components/Contact/index.js | Migrated from Contact.defaultProps to default parameters in function signature |
| packages/app-rfi/src/components/controls/controls-helpers.js | Migrated helper components from defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiTextInput.js | Migrated from RfiTextInput.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiTextArea.js | Migrated from RfiTextArea.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiSelect.js | Migrated from RfiSelect.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiPhone.js | Migrated from RfiPhone.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiEmailInput.js | Migrated from RfiEmailInput.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiDateInput.js | Migrated from RfiDateInput.defaultProps to default parameters |
| packages/app-rfi/src/components/controls/RfiCheckboxSingle.js | Migrated from RfiCheckboxSingle.defaultProps to default parameters |
| packages/app-rfi/src/components/AsuRfi/index.js | Migrated from AsuRfi.defaultProps to default parameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
|
|
||
| const Social = ({ social: { logoUrl, unitLogo, mediaLinks } }) => { | ||
| const Social = ({ social: { logoUrl, unitLogo = endorsedLogo, mediaLinks = {facebook: "", twitter: "", linkedIn: "", instagram: "", youtube: ""} } }) => { |
There was a problem hiding this comment.
The propTypes definition marks unitLogo as isRequired, but the destructuring now provides a default value. This creates a conflict between the validation and the actual behavior. Either remove isRequired from the PropType at line 158, or restructure the code to handle defaults without nested destructuring.
| const Social = ({ social: { logoUrl, unitLogo = endorsedLogo, mediaLinks = { | ||
| facebook: "", | ||
| twitter: "", | ||
| linkedIn: "", | ||
| instagram: "", | ||
| youtube: "", | ||
| } } }) => { |
There was a problem hiding this comment.
The propTypes definition marks unitLogo as isRequired, but the destructuring now provides a default value. This creates a conflict between the validation and the actual behavior. Either remove isRequired from the PropType at line 169, or restructure the code to handle defaults without nested destructuring.
| */ | ||
|
|
||
| const Social = ({ social: { logoUrl, unitLogo, mediaLinks } }) => { | ||
| const Social = ({ social: { logoUrl, unitLogo = endorsedLogo, mediaLinks = {facebook: "", twitter: "", linkedIn: "", instagram: "", youtube: ""} } }) => { |
There was a problem hiding this comment.
The inline object with multiple properties reduces readability. Consider defining the default mediaLinks object as a constant outside the function, similar to how DEFAULT_GA_EVENT is defined at line 17-20.
There was a problem hiding this comment.
@oatkar This is actually a decent suggestion from copilot. Can you create a separate const with those values in medialinks and just assign it to mediaLinks
| options, | ||
| disabled, | ||
| autoFocus, | ||
| disabled = undefined, |
There was a problem hiding this comment.
The original defaultProps specified disabled: false, but the new default is undefined. This changes the component behavior when disabled is not provided. The default should be false to maintain backward compatibility.
| disabled = undefined, | |
| disabled = false, |
Co-authored-by: Copilot <[email protected]>
davidornelas11
left a comment
davidornelas11
left a comment
There was a problem hiding this comment.
Once change requested by copilot
Description
Refactoring defaultProps to use JS default paramsChecklist
Important Reminders
Links