search

nickmcmillan / react-pig

Arrange images in a responsive, progressive-loading grid managed in JavaScript using CSS transforms.
56 stars 11 forks source link

Add the option to supply your own handleClick function #37

Open derneuere opened 3 years ago

derneuere commented 3 years ago

This PR adds the option to supply your own handleClick function as a prop. It also contains a fix for the package.json. Yarn tries to fetch es-abstract 1.14.0, which does not exist. By adding version 1.14.1 to the dependencies, it now builds again.

nickmcmillan commented 3 years ago

It's been about 2 years since I've touched this project so haven't tried to run it locally for a long time. Was it not running locally for you? I presume that's why in this PR you've included babel and rollup config changes.

You'll have to make changes before I can review this PR properly. You've included your own styling changes - probably done automatically by your IDE - which have converted all single quotes to double, added semicolons everywhere, and added various additional line breaks. I'd recommend against attempting to change the coding style of a project you contribute to.

There's multiple console logs included like "Enemy spotted!", "We are going in". Are these relevant to the library you're contributing to? If not they shouldn't be in a PR.

You should try using let instead of var when declaring variables which you plan on modifying.

This PR appears to include functionality to select multiple items. That doesn't match up with what the title or the description of this PR says. Could you try to focus the PR on one feature? If you plan on adding multiple selection functionality, what would the API look like for the end-user of this library? It seems you've added selectable as a boolean prop - but what of the actual selected items?

There also seems to be transform interpolations applied to margin top/left/bottom/right. This should be avoid for repaint/reflow issues. Sometimes it's unavoidable - I can see I've used it on width/height. But I don't want to introduce more expensive operations without good cause. Maybe you could use css outline or box-shadow to apply a "selected" visual style instead?

I'm glad that you found a use for this library and I appreciate you helping out. Sorry if this is a bit too much feedback, but I'm hoping it points you in the right direction.

derneuere commented 3 years ago

Sorry, for the confusion. I accidentally pushed work in progress changes to this branch instead to my other one. I will create two other pull requests: One with updates to the dependencies including babel and rollup config changes and the another one for selecting images.

It looks like I have set up the wrong formatter. Which one should I use?

I only had to add es-abstract 1.14.1 to the dependencies to get it to run. In the other commit, I updated the rest of the dependencies just to have the project to be up-to-date.

Thank you for this awesome library :)