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

Redux for experiments table drag and drop #2097

Merged
merged 10 commits into from
Jul 26, 2022
Merged

Redux for experiments table drag and drop #2097

merged 10 commits into from
Jul 26, 2022

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Jul 25, 2022

As this reducer already existed, it was easy to remove the DragDropContext and just use the reducer. Things seem to work just as they did for drag and drop. @wolmir can you test and tell me if I missed anything. All tests were passing, so it should be fine, but we never know...

Starting work for #2103

@sroy3 sroy3 self-assigned this Jul 25, 2022
@sroy3 sroy3 changed the base branch from main to plots-redux-naming July 25, 2022 20:03
@sroy3 sroy3 marked this pull request as ready for review July 25, 2022 20:11
@sroy3 sroy3 marked this pull request as draft July 25, 2022 20:23
@sroy3 sroy3 marked this pull request as ready for review July 25, 2022 20:26
Base automatically changed from plots-redux-naming to main July 25, 2022 21:18
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.

Looks good to me!

@codeclimate
Copy link

codeclimate bot commented Jul 26, 2022

Code Climate has analyzed commit d6ee560 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 88.0% (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.

@wolmir
Copy link
Contributor

wolmir commented Jul 26, 2022

Sorry @sroy3 I'll test it now

Copy link
Contributor

@wolmir wolmir left a comment

Choose a reason for hiding this comment

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

Works great, thanks!

Comment on lines +206 to +210
<Provider store={configureStore({ reducer: experimentsReducers })}>
<div style={{ height: 400, width: 600 }}>
<Experiments tableData={tableData} />
</div>
</Provider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Provider could be added with a decorator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good idea, I'll add this in my follow up PR

@sroy3 sroy3 merged commit 1f4c831 into main Jul 26, 2022
@sroy3 sroy3 deleted the experiments-redux-dnd branch July 26, 2022 17:20
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.

4 participants