Skip to content
This repository was archived by the owner on May 17, 2021. It is now read-only.

Conversation

lrlna
Copy link
Contributor

@lrlna lrlna commented Nov 6, 2019

Description

Current work going into being able to Select Fields when exporting.

So far this PR:

  • separates the three stages of exporting: select query vs. full collection, selecting fields, selecting output
  • keeps error message throughout all stages and will render above the Back, Close, {PROCEED} buttons
  • fields selected are kept in props, so when a user goes back to selecting fields, their previously selected/disselected fields are still there
  • if a user goes back to SELECT FIELDS modal state after having exported, they can edit fields, and export again (dunno if anyone will actually do this, but just in case)

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Open Questions

Currently I am finding one document and grabbing the initial fields to render from that document. Should I instead do a collection sample and run schema-parser over that to get a more accurate selection of fields. I can't determine if that's an overkill, or, in fact, a more detailed solution.

Help! I can't figure out why my export-select-fields component is not rerendering. At this point I am being very impolite towards React, but I am sure there is a simple thing I am not seeing.

I am thinking I will do + ADD FIELD button in another PR, so people can play around with this for now and tell me if everything works ok.

Types of changes

  • Minor (non-breaking change which adds functionality)

@lrlna
Copy link
Contributor Author

lrlna commented Nov 7, 2019

@imlucas this is the PR that has the three step flow for export!

Copy link
Contributor

@imlucas imlucas left a comment

Choose a reason for hiding this comment

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

@imlucas this is the PR that has the three step flow for export!
@lrlna awesome thanks!

@lrlna lrlna requested a review from imlucas November 15, 2019 16:38
@lrlna lrlna changed the title [WIP] COMPASS-3851: ability to add fields when exporting collection COMPASS-3851: ability to add fields when exporting collection Nov 15, 2019
Copy link
Contributor

@imlucas imlucas left a comment

Choose a reason for hiding this comment

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

Currently I am finding one document and grabbing the initial fields to render from that document. Should I instead do a collection sample and run schema-parser over that to get a more accurate selection of fields. I can't determine if that's an overkill, or, in fact, a more detailed solution.

I've been thinking the same for import

Help! I can't figure out why my export-select-fields component is not rerendering. At this point I am being very impolite towards React, but I am sure there is a simple thing I am not seeing.

Let's pair up. Nothing sticking out cause.

I am thinking I will do + ADD FIELD button in another PR, so people can play around with this for now and tell me if everything works ok.

Sounds good

@lrlna
Copy link
Contributor Author

lrlna commented Nov 18, 2019

Currently I am finding one document and grabbing the initial fields to render from that document. Should I instead do a collection sample and run schema-parser over that to get a more accurate selection of fields. I can't determine if that's an overkill, or, in fact, a more detailed solution.

I've been thinking the same for import

Hang on, so are you sampling or doing a find one?

Thanks for a kick-ass review, btw!

@imlucas
Copy link
Contributor

imlucas commented Nov 18, 2019

@lrlna

Hang on, so are you sampling or doing a find one?
Doing equivalent of findOne() today → adding a todo to think about schema/sampling someday if that ends up being too limited

@imlucas
Copy link
Contributor

imlucas commented Nov 18, 2019

Oh and I was thinking more about "export-select-fields component is not rerendering" last night. If this is still a problem w/ not rerendering make it an array of includeFields: ['_id', 'foo', etc...] then convert it into an object in your reducer when passing it to data-service.

// }

handleFieldCheckboxChange = (evt) => {
const fields = Object.assign({}, this.props.fields);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imlucas solved the re-render issue, and oh my god was is it the most nuanced react thing.

so what i had before here was:

const fields = this.props.fields;

which I then modified and updated the redux state with. So this is considered a mutation by redux, so it will just give up and not fire a re-render apparently \o/

using Object.assign() gets rid of the mutation and creates a brand new thing i can rewrite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Sweet! Nice find

@lrlna
Copy link
Contributor Author

lrlna commented Nov 20, 2019

@imlucas i fixed everything we talked about IRL yesterday, let me know if it looks good to merge!

@imlucas imlucas merged commit 35d2722 into master Nov 20, 2019
@imlucas imlucas deleted the COMPASS-3851 branch November 20, 2019 14:10
@imlucas imlucas mentioned this pull request Dec 4, 2019
imlucas added a commit that referenced this pull request Dec 4, 2019
Cutting master so all of your export improvements from the past few days can be upstreamed into compass asap with @mongodb-js/compass-import-export@^4.2.0.

The import type casting stuff in #18 is complex enough that I don't want it blocking so it will go as 5.0.0. We can keep this 4.x-releases branch ref around just in case casting performance is a real dud and we want to take another stab at it.

Now that this release has been published, a PR to mongodb-js/compass 📦 @mongodb-js/compass-import-export@^4.2.0 has changelog:

UX improvements: display doc count, do not display line numbers #20
COMPASS-3961: allow adding missing fields #19
COMPASS-3851: ability to add fields when exporting collection #17
imlucas added a commit to mongodb-js/compass that referenced this pull request Dec 6, 2019
Now that this release has been published, a PR to mongodb-js/compass 📦 `@mongodb-js/compass-import-export@^4.2.0` has changelog:

- UX improvements: display doc count, do not display line numbers mongodb-js/compass-import-export#20
- Closes COMPASS-3961: allow adding missing fields mongodb-js/compass-import-export#19
- Closes COMPASS-3851: ability to add fields when exporting collection mongodb-js/compass-import-export#17
imlucas added a commit to mongodb-js/compass that referenced this pull request Dec 6, 2019
)

Now that this release has been published, a PR to mongodb-js/compass 📦 `@mongodb-js/compass-import-export@^4.2.0` has changelog:

- UX improvements: display doc count, do not display line numbers mongodb-js/compass-import-export#20
- Closes COMPASS-3961: allow adding missing fields mongodb-js/compass-import-export#19
- Closes COMPASS-3851: ability to add fields when exporting collection mongodb-js/compass-import-export#17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants