-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat(app,shared-data): Add basic support for the Flex Stacker module to the front-end. #17295
Conversation
057a705
to
2e98eb4
Compare
export const FlexStackerModuleData = ( | ||
props: FlexStackerModuleProps | ||
): JSX.Element | null => { |
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.
export const FlexStackerModuleData = ( | |
props: FlexStackerModuleProps | |
): JSX.Element | null => { | |
export function FlexStackerModuleData( | |
props: FlexStackerModuleProps | |
): JSX.Element | null { |
onCloseClick={handleCloseSlideout} | ||
isExpanded={isExpanded} | ||
> | ||
<LegacyStyledText |
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 there any specific reason to use LegacyStyledText
instead of StyledText
?
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'd guess this is copy/pasted from older module slide outs. I'll update it
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 recommend adding tests
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'll make a TODO to add tests when we build this out to design specs. This PR is just meant to unblock science & ABR from using the stacker in a protocol!
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 recommend adding tests
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.
Tested this out in the app & ODD - changes look good as a first pass to unblock science & ABR
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17295 +/- ##
==========================================
+ Coverage 73.84% 79.04% +5.20%
==========================================
Files 43 120 +77
Lines 3303 4587 +1284
==========================================
+ Hits 2439 3626 +1187
- Misses 864 961 +97
Flags with carried forward coverage won't be shown. Click here to find out more. |
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 changes look good to me
Overview
This adds the basic scaffolding for the Flex Stacker so the app won't crash when it encounters one.
Closes: EXEC-1088 EXEC-1089
Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment