Refactoring image selector components

[evanzhong]

Profile Picture Selector

Edit Sketch Selector

Create Sketch Selector

Props to be passed into ImageSelector components:
isOpen: boolean for whether the modal is open
closeModal: callback for when the modal is closed
icons: array of elements for icons to display
maxWidth: optional limit on the width of the modal
thumbnailPreview: optional image element to display on the header showing what icon the user has currently selected
error: optional error state
children: optional footer buttons

[mattxwang]

Hey @evazhog, the PR fails one of our tests (specifically it's one on the profile panel image selector, which makes sense) - do you mind fixing the test to use the new structure of the image selector?

Other than that, this looks generically good; I'll give it a more thorough look over in a moment.

[mattxwang]

Overall the functionality looks good, but left a bit of review in terms of possible app structure ideas.

[mattxwang]

Hey Evan, thanks for taking the time to sit down with me and work through on this ticket! We figured out (some) of the issues with the broken tests, moved around some code, and added some constants.

Just to recap, a few things that need to be done for the rest of the ticket:

• write an ImageSelector.scss file that contains all the styling for ImageSelector.js - ideally, we want no leakage from other styles (in case they change in the future)
• write an ImageSelector.test.js file that tests the component by itself
  • write a smoke test: check that the component renders shallow-ly
  • write some props tests: check that the component handles each of its props properly. edge-cases aren't too important here, but make sure that you cover the generic inputs they'll receive (especially because usually we pass in variables to these components, instead of literals)
  • bonus: write some mouse-event simulator tests: selecting each element and checking the state change, checking if clicking the button triggers the function (you can look into mock jest functions for this), etc. you can look at other test files and see how they do this

[mattxwang]

Just for sake of clarity: all of the necessary refactoring is done, great job @evazhog! Waiting on possible extra tests for the custom component, but at the current state this LGTM.

[krashanoff]

Just checking in, @evazhog, are you planning on adding any tests to your refactor? If not, I am happy to merge this and write tests after the fact. Just wanted to follow up as this PR has been in limbo for a while now.

[evanzhong]

Plan on taking another pass at it tomorrow, can't get to it tonight unfortunately.

[krashanoff]

No worries Evan! Just wanted to check in. Take it easy!

[krashanoff]

On account of core functionality being OK, I'll be going ahead and merging this. Seeing as this component favors integration tests, we can always add them down the line.

Thanks for all your work Evan!