New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use table-based user cover presets #11116
base: master
Are you sure you want to change the base?
Conversation
|
||
import UserCoverPresetBatchActivate from 'user-cover-preset-batch-activate'; | ||
|
||
new UserCoverPresetBatchActivate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructing an object and not assigning it is weird as it implies there's no reference to it and it should get gc'd but it doesn't from the global handlers in it (also smells like using new
to create side-effect). Use a static initializer and private constructor instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you reviewing something from previous pr ಠ_ಠ
@@ -29,6 +28,23 @@ export default class CoverSelector extends React.Component<Props> { | |||
private readonly eventId = `users-show-cover-selector-${nextVal()}`; | |||
private readonly uploaderRef = React.createRef<CoverUploader>(); | |||
|
|||
@computed | |||
private get holdoverCoverPreset(): UserCoverPresetJson|null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing spaces
<CoverSelection | ||
confirmUpdate={confirmUpdate} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just false
? or default false
use App\Transformers\UserCoverPresetTransformer; | ||
use Illuminate\Support\Collection; | ||
|
||
class UserCoverPresets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if it's worth calling resetMemoized()
when the covers are updated in the controller?
🤷