-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor(app, components): parametersTable moved to Components dir for PL #14734
Conversation
…rectory for PL closes AUTH-240
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14734 +/- ##
=======================================
Coverage 67.18% 67.18%
=======================================
Files 2495 2495
Lines 71651 71651
Branches 9075 9075
=======================================
Hits 48140 48140
Misses 21367 21367
Partials 2144 2144
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.
can the folder be named ParametersTable
to match the current pattern (see CheckboxField
)
@shlokamin oh ya, i was following the other folders at the |
app/src/organisms/Devices/ProtocolRun/__tests__/ProtocolRunRuntimeParameters.test.tsx
Outdated
Show resolved
Hide resolved
app/src/organisms/Devices/ProtocolRun/__tests__/ProtocolRunRuntimeParameters.test.tsx
Outdated
Show resolved
Hide resolved
|
||
interface NoParametersProps { | ||
t?: any |
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.
if the type in utils.test.ts, probably using that instead of any would be good.
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 type of t
comes from useTranslation
but components doesn't have i18Next
as a dependency, so i left it typed as any
. Idk if there are any better methods? Do you have suggestions?
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.
at this moment no, I will keep an eye on that.
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.
cool thanks, same here. I also don't like typing it as any
but i'm not sure how else to do it so the app is still translated.
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.
lgtm
closes AUTH-240
Overview
This PR moves the parameters desktop app table and empty state from the app directory to the components directory and also moves the formate value util to shared data
Test Plan
In the desktop app, check the protocol details parameters tab and make sure it renders as expected
Changelog
parametersTable
and move the no parameters component in that folder as wellShared-data
and fix all instances of itt
as a prop for the app since it uses i18n but PL does notReview requests
see test plan
Risk assessment
low