Skip to content

Conversation

@VeraZab
Copy link
Contributor

@VeraZab VeraZab commented May 23, 2018

No description provided.

@VeraZab VeraZab force-pushed the adjust-src-format branch 10 times, most recently from a8d9506 to 046ecef Compare May 24, 2018 20:19
if (
this.is2D &&
this.fullValue &&
this.fullValue.length &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

guards against empty arrays

if (config.deleteKeys && !(srcRef in dataSources)) {
delete parent[dataKey];
return;
// making this into an array to more easily lookup 1d and 2d srcs in dataSourceOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's easier to have this in an array format to lookup srcs in dataOptions, this behaviour here was broken in current dereferencing function for array srcs

@VeraZab
Copy link
Contributor Author

VeraZab commented May 24, 2018

@bpostlethwaite ready for review

@nicolaskruchten
Copy link
Contributor

OK so on the src-handling front, the API is a bit murky for me... Right now we expect srcs to either be strings or arrays of strings, correct? And we want to provide a pair of functions that allow someone integrating the editor to be able to pass in some other format (i.e. encoded strings like we have in streambed) and the joinSrc and splitSrc functions are mappings between what the editor wants to process and what is externally needed. So I would rename those toSrc and fromSrc or something, to make it a bit clearer what the API is.

@nicolaskruchten
Copy link
Contributor

I would also have those functions be distinct props, rather than keys in an opaque-looking customSrcHandling object prop :)

@VeraZab
Copy link
Contributor Author

VeraZab commented May 25, 2018

Thanks @nicolaskruchten ! Made the adjustments.

I think I like keeping the customSrcHandling object because I think it's maybe clearer that both toSrc and fromSrc are needed if we're going to customSrcHandling route. I added Proptypes.shape to make it more explicit what that object contains.

It's ready for a second look when you have a chance.

@VeraZab VeraZab force-pushed the adjust-src-format branch 2 times, most recently from 2945268 to b62cf33 Compare May 25, 2018 20:47
@nicolaskruchten
Copy link
Contributor

💃 from me on the high-level API. The tests seem good to me.

@nicolaskruchten
Copy link
Contributor

sorry, hit wrong button :)

@VeraZab VeraZab force-pushed the adjust-src-format branch from b62cf33 to cf5c799 Compare May 28, 2018 16:42
@VeraZab VeraZab merged commit 9c2dafb into master May 28, 2018
@VeraZab VeraZab deleted the adjust-src-format branch May 28, 2018 16:45
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