Skip to content

Conversation

@sroy3
Copy link
Contributor

@sroy3 sroy3 commented Jul 27, 2022

Closes #2103

@sroy3 sroy3 self-assigned this Jul 27, 2022
@sroy3 sroy3 marked this pull request as ready for review July 27, 2022 20:55
...state,
...action.payload,
hasData: true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty much the same as it was before. An improvement here would be to go through previous columns, rows and objects to change the reference of only what changed. I will do that as a follow up.

width: DEFAULT_COLUMN_WIDTH
}

export const ExperimentsTable: React.FC = () => {

Choose a reason for hiding this comment

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

Function ExperimentsTable has 73 lines of code (exceeds 30 allowed). Consider refactoring.

}

export const ExperimentsTable: React.FC = () => {
const {

Choose a reason for hiding this comment

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

Similar blocks of code found in 3 locations. Consider refactoring.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work! Things look more readable with less props being passed around :)

Copy link
Contributor

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

[Q] How does performance compare? Looks good to go otherwise.

@sroy3
Copy link
Contributor Author

sroy3 commented Jul 28, 2022

[Q] How does performance compare? Looks good to go otherwise.

I'm not seeing any difference. It's pretty much just a drop in replacement since the whole thing changes when the data changes. Like I said I'll follow up to try to limit the changes to only what changes. That could help with really big project, but that is not something we will notice otherwise.

@sroy3 sroy3 enabled auto-merge (squash) July 28, 2022 13:26
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit ddd0779 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 3

The test coverage on the diff in this pull request is 96.2% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 merged commit dfb8ead into main Jul 28, 2022
@sroy3 sroy3 deleted the experiments-redux branch July 28, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Redux to experiments table

3 participants