Conversation
|
Thank you for your PR! @evank28 what do you think? |
evank28
left a comment
evank28
left a comment
There was a problem hiding this comment.
Thanks for doing this!
Do you think it would be worth trying to adopt the Meta approach directly?
They released an NPM package that should get you sepia, blur, and a whole bunch of other filters with an import statement -- https://www.npmjs.com/package/content-review-filters?activeTab=code
@lswartsenburg did some work trying to demo how you can use the NPM package in your own react app here: https://github.com/lswartsenburg/run-content-filters
| moderatorSafetyBlurLevel: BlurStrength; | ||
| moderatorSafetyGrayscale: boolean; | ||
| moderatorSafetyMuteVideo: boolean; | ||
| moderatorSafetySepia: boolean; |
There was a problem hiding this comment.
One thing to consider -- the approach open sourced by Meta suggests letting users have different preference settings for different types of content (https://github.com/facebook/content-review-filters/blob/main/packages/content-review-components/ContentReviewFilterGlobalPreferenceContext.tsx#L37). So, a user could have Sepia enabled for animal abuse, for example, but not for graphic violence. At some point it may make sense to refactor the data model to take this into account.
You might also want to adopt a React Context approach like shown in that link, rather than having to pass the state down via props.
| ...prevSettings, | ||
| moderatorSafetySepia, | ||
| })), | ||
| [], |
There was a problem hiding this comment.
Consider abstracting away this logic so its a single callback that takes the name of the param being changed, so you don't need to create as many separate callbacks.
| moderatorSafetyBlurLevel: 2, | ||
| moderatorSafetyGrayscale: true, | ||
| moderatorSafetyMuteVideo: true, | ||
| moderatorSafetySepia: true, |
| : 'object-scale-down grow max-w-64 max-h-48' | ||
| } ${safetySettings.moderatorSafetyGrayscale ? 'grayscale' : ''}`} | ||
| } ${safetySettings.moderatorSafetyGrayscale ? 'grayscale' : ''} ${ | ||
| safetySettings.moderatorSafetySepia ? 'sepia' : '' |
There was a problem hiding this comment.
I guess in the future you would look at parameterizing the intensity?
3 participants
Context & Requests for Reviewers
Implement Sepia filter from #43 using https://tailwindcss.com/docs/filter-sepia
TODO: need to run lint
Tests
Only tested on account settings page
(Optional) Rollout Plan