Skip to content
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

[feat] Allow rename before image upload in EditPage, refactor ImageModal #117

Merged
merged 2 commits into from
Dec 26, 2019

Conversation

kwajiehao
Copy link
Contributor

@kwajiehao kwajiehao commented Dec 24, 2019

Overview

This PR achieves a couple of things:

  1. It allows the user to rename an image before uploading an image in the EditPage component (previously implemented for the Images tab in [Feat] Allow image upload in EditPage #105).
  2. It refactors the ImageModal component to be a React PureComponent. This provides performance benefits when rendering newly uploaded images. We can use PureComponent because the shallow comparison performed by PureComponent (which only compares references for objects/nested state) is sufficient because we do not and will not allow users to replace the content of an image while retaining the file name.

Renaming before upload

This functionality is facilitated by storing images in the parent EditPage instead of inside ImageModal. If images were stored inside ImageModal, we would need to define a modal inside a modal, which would prevent us from taking advantage of the performance benefits afforded by using PureComponent. The flow for uploading an image in EditPage thus becomes:

  • Click on Image button in editor. This triggers a call to retrieve images and then toggle so that ImageModal appears
  • Click on upload new image in ImageModal. This allows the user to select an image to upload.
  • Select the image to upload. Upon selection, the user is faced with the ImageSettingsModal to rename the image. Upon clicking save, EditPage retrieves the images again so that the newly uploaded image is reflected in ImageModal and the ImageSettingsModal is closed.

This commit achieves two things:

1. It allows ImageSettingsModal to receive a `toReload` prop since
we do not always want the page to reload. In the case of EditPage
with the ImageModal, we want users to be able to upload an image
and then select it right away. If we do not have a `toReload` prop,
the window will reload and the ImageModal will be reset.

2. It adds custom logic for `isPendingUpload`. In cases where the
image/file is pending upload, there should not be a delete button
in the ImageSettingsModal, and the Save button would not be subject
to the usual constraints (requiring a `sha` value before the button
becomes active since the create operation does not require a `sha`)
This commit accomplishes a couple of things:

1. It introduces the functionality to rename image before uploading for the
ImageModal (this functionality was firrst introduced for the Images tab in
PR #107).

2. It refactors the ImagesModal to be a PureComponent. Previously, the ImageModal
retrieved images and saved this images in state. This functionality can be moved
to its parent component, EditPage, so that only an `images` prop is passed to the
ImageModal.

This is beneficial because the ImageModal logic is simple and its output really
depends on the images that are passed as props to it. Since the key for the images
are the filenames, this means that when we upload a new image, React will perform
a shallow compare based on the filenames and only render the new uploaded image
instead of re-rendering all the images.
@kwajiehao kwajiehao requested a review from taufiq December 24, 2019 10:29
Copy link
Contributor

@prestonlimlianjie prestonlimlianjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm.

Adding 2 issues related to this PR:
#118
#119

@kwajiehao kwajiehao merged commit 08756ee into staging Dec 26, 2019
@kwajiehao kwajiehao deleted the feat/rename-image-on-upload-edit-page branch December 26, 2019 06:45
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.

2 participants