Skip to content
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): add Parameters tab to odd protocolDetails #14657

Merged
merged 8 commits into from
Mar 15, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 14, 2024

closes AUTH-113, AUTH-124, AUTH-115, AUTH-114

Overview

Creates the parameters tab in the ProtocolDetails section of the ODD also modify the empty section component for parameters

Test Plan

go to protocol details on the odd and see that the parameters tab exists. clicking on the tab should display the empty parameters component. Observe the Parameters page and click on it to make sure the snackbar appears.

Changelog

  • add support to EmptySection for parameters
  • create new Parameters component and test
  • add Parameters as an option in the types to add it as a tab
  • add snackbar to the parameters component

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner March 14, 2024 13:18
@jerader jerader requested review from ncdiehl11 and removed request for a team March 14, 2024 13:18
@@ -256,6 +258,9 @@ const ProtocolSectionContent = ({
/>
)
break
case 'Parameters':
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure an easy way to put this behind the ff? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new option in protocolSectionTabOptions should be behind a FF. you can either make a new constant or conditionally spread it in to the list. but as long as you do that you dont need to put stuff in ProtocolSectionContent cuz it wont be possible to select the params tab

@jerader jerader requested review from koji and a team March 14, 2024 13:19
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.34%. Comparing base (574f793) to head (2a0190b).
Report is 2 commits behind head on edge.

❗ Current head 2a0190b differs from pull request most recent head 2fc81dd. Consider uploading reports for the commit 2fc81dd to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14657      +/-   ##
==========================================
+ Coverage   67.33%   67.34%   +0.01%     
==========================================
  Files        2485     2485              
  Lines       71386    71416      +30     
  Branches     9030     9041      +11     
==========================================
+ Hits        48069    48098      +29     
  Misses      21173    21173              
- Partials     2144     2145       +1     
Flag Coverage Δ
shared-data 75.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
app/src/pages/ProtocolDetails/EmptySection.tsx 100.00% <ø> (ø)
app/src/pages/ProtocolDetails/index.tsx 62.93% <ø> (ø)

... and 43 files with indirect coverage changes

@jerader jerader force-pushed the app_parameters-protocol-details-tab branch from 2a0190b to a879479 Compare March 14, 2024 18:49
range = `${parameter.choices[0].displayName}, ${parameter.choices[1].displayName}`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are right. we just need to handle 2 choices this can be better than what I did.

@jerader jerader force-pushed the app_parameters-protocol-details-tab branch from 99121da to aded36c Compare March 15, 2024 17:57
}

return runTimeParameters.length > 0 ? (
<Table onClick={makeSnack} data-testid="Parameters_table">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was fine when we talked about this yesterday's meeting.
But now I think this might show the snack bar unexpectedly.

Right now I'm having the issue of ssh to a dev kit and flexs so not 100% sure but I guess this implementation may show the snack bar when scrolling the lists?

If that doesn't happen, this implementation is okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i might merge this in to not block you but i think you're right that this will cause an issue, i'll post in slack about it. Maybe we need it to appear during a long press

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the scroll doesn't cause the issue I mentioned, the changes make sense to me.

@jerader jerader merged commit 5efa903 into edge Mar 15, 2024
20 checks passed
@jerader jerader deleted the app_parameters-protocol-details-tab branch March 15, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants