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

refactor(protocol-designer): remove usage of class components and connect fns #14331

Merged
merged 20 commits into from
Jan 25, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jan 19, 2024

closes RAUT-935

Overview

This PR refactors out the usage of the majority of connect functions and all class components. The connect functions I left were because they use react-dnd

Test Plan

smoke test Pd! make sure things work as expected and that you don't run into any white screens or buttons that don't work.

Changelog

  • removed all usages of class components
  • removed many instances of connect functions
  • fix some tests

Review requests

see test plan

Risk assessment

medium-ish, touches a lot of code but there is no release coming anytime soon

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 370 lines in your changes are missing coverage. Please review.

Comparison is base (6b725a2) 68.02% compared to head (1ab6f3a) 68.08%.
Report is 16 commits behind head on edge.

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14331      +/-   ##
==========================================
+ Coverage   68.02%   68.08%   +0.05%     
==========================================
  Files        2505     2498       -7     
  Lines       71556    71540      -16     
  Branches     9082     9086       +4     
==========================================
+ Hits        48679    48710      +31     
+ Misses      20767    20713      -54     
- Partials     2110     2117       +7     
Flag Coverage Δ
protocol-designer 35.29% <7.50%> (+0.40%) ⬆️

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

Files Coverage Δ
...rotocol-designer/src/components/ProtocolEditor.tsx 0.00% <ø> (ø)
...nents/StepEditForm/fields/WellOrderField/index.tsx 4.34% <ø> (ø)
protocol-designer/src/localization/index.ts 12.50% <ø> (+1.38%) ⬆️
...m/fields/WellSelectionField/WellSelectionModal.tsx 6.25% <0.00%> (ø)
...ponents/steplist/StartingDeckStateTerminalItem.tsx 0.00% <0.00%> (ø)
...col-designer/src/components/SettingsPage/index.tsx 0.00% <0.00%> (ø)
...mponents/DeckSetup/LabwareOverlays/LabwareName.tsx 0.00% <0.00%> (ø)
...col-designer/src/containers/ConnectedMainPanel.tsx 0.00% <0.00%> (ø)
.../FileUploadMessageModal/FileUploadMessageModal.tsx 0.00% <0.00%> (ø)
...tocol-designer/src/containers/ConnectedSidebar.tsx 0.00% <0.00%> (ø)
... and 24 more

@jerader jerader marked this pull request as ready for review January 22, 2024 22:39
@jerader jerader requested a review from a team January 22, 2024 22:39
@koji koji requested a review from a team January 23, 2024 15:48
@jerader jerader requested a review from koji January 23, 2024 18:25
@jerader jerader requested a review from a team January 24, 2024 14:04
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

Smoke test and a cursory pass through looks good. This was a lot of work, well done! 🚀

Just curious, why were some of these written as arrow functions?


const missingTipsByLabwareId = tipContentsSelectors.getMissingTipsByLabwareId(
state
export const LabwareOnDeck = (props: LabwareOnDeckProps): JSX.Element => {
Copy link
Contributor

@koji koji Jan 24, 2024

Choose a reason for hiding this comment

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

As Jamey pointed out

Suggested change
export const LabwareOnDeck = (props: LabwareOnDeckProps): JSX.Element => {
export function LabwareOnDeck = (props: LabwareOnDeckProps): JSX.Element {

I think we follow airbnb styling guidelines and I'm not sure about pd but in app we utilize hoisting to create a component inside component.
https://airbnb.io/javascript/react/
 

// bad
class Listing extends React.Component {
  render() {
    return <div>{this.props.hello}</div>;
  }
}

// bad (relying on function name inference is discouraged)
const Listing = ({ hello }) => (
  <div>{hello}</div>
);

// good
function Listing({ hello }) {
  return <div>{hello}</div>;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you for this Koji! Very helpful. I'll refactor.

@jerader
Copy link
Collaborator Author

jerader commented Jan 25, 2024

Just curious, why were some of these written as arrow functions?

@mjhuff, ah good catch, i'm reading through Koji's comments. I'll refactor them! I'll admit that i wasn't really sure of the differences 😓 but it is a good idea to be consistent across PD and the app

@jerader jerader merged commit 30d6876 into edge Jan 25, 2024
14 checks passed
@jerader jerader deleted the pd_refactor-connect-fn branch January 25, 2024 14:04
ncdiehl11 pushed a commit that referenced this pull request Feb 1, 2024
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