Skip to content

Conversation

yoanribot
Copy link

First of all, nice works ;). I've made a little improvement to the headers prop, for cases where your data collection contain nested objects ( for now was only working for plain objects) where basically I use the lodash / get method to access the key(data index) in the object to allow cases like:

 this.exportColumns = [
      { label: 'Name', key: 'name' },
      { label: 'Ignored', key: 'calibrationStatus.ignored' },
      { label: 'Deleted', key: 'calibrationStatus.deleted' },
      { label: 'Merged / Group', key: 'calibrationStatus.mergeRoot' },
       ...
    ];

I hope you like the idea ;)

@coveralls
Copy link

coveralls commented Jun 21, 2018

Coverage Status

Coverage increased (+0.07%) to 94.872% when pulling 82d6e08 on yoanribot:support-nested-object-key into e7cdb55 on abdennour:master.

Yoan Ribot and others added 5 commits June 22, 2018 13:04
Updates node-sass package
Update package json
Adds preinstall script
Loads only lodash/get function
@mccabemj
Copy link
Collaborator

@yoanribot Didn't see this PR, so it was done in PR #107 to fix issue #84 which was outstanding.

Your solution is smaller (even though you have not provided tests and build failed). However, it adds a dependency for Lodash, where I don't see much benefit for such a small portion of the package. @abdennour perhaps you can lend your thoughts

@yoanribot
Copy link
Author

Sorry for the unit test. Basically, my idea is to use the _get function of lodash. You don't have to import the entire library, just what you need:
var _get = require('lodash/get');

As you can see you are going to import only one function but if you think that is a problem (you could do your own version of the get of lodash, what I think is not a good idea because you should avoid reinvent the wheel)

@mccabemj
Copy link
Collaborator

@yoanribot I don't actually think it does. Can't see any tree shaking going on with Babel, so its increased the minified package size by 15% and the un-minified by about 30%. Unless I am missing something here?

Unless I am wrong with the above, you could bring in the _get of lodash by another means, but increasing the package size by that much for such a small feature seems wasteful.

If you can get the package size down, write tests, then remove some un-needed things (node-sass version increase, and a new install script?) I'd choose this over my implementation

@yoanribot
Copy link
Author

yes sure, maybe in the future when I have some time. For now, I'm fine with my fork :).

@yoanribot
Copy link
Author

BTW your solution(Added in capability for dot notation in headers) is not merged yet, there are some conflicts pending to resolve. Could be cool if someone could take care of that if have a time of course :)

@mccabemj
Copy link
Collaborator

@yoanribot yeah, I was waiting to discuss this PR with you. I'll merge #107 for now and close this PR. If you get time to solve the package size issue with lodash and want to re-open, feel free. thanks for you help, and if you fancy jumping on any other issues, help would be great ;-)

@mccabemj mccabemj closed this Oct 19, 2018
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