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

Fix/clean up of ComponentSettingsModal and OverviewCard #385

Closed
wants to merge 7 commits into from

Conversation

kwajiehao
Copy link
Contributor

@kwajiehao kwajiehao commented Mar 23, 2021

Context

Over the past couple of months, we've made some strategic decisions which have rendered some parts of the codebase irrelevant.

1. Our move to use Jekyll 4, and using collection config files to determine collection/folder page order

Previously, since collection page order was determined by the file name prefixes, we had complicated util functions (generateNewCollectionFileName, generateGroupIdentifier, etc.) which helped us to calculate the file names when we create, delete, update, or move collection pages. With the new collection config files, these util functions are no longer needed since we no longer need to generate file name prefixes.

2. Our decision to more closely mimic Google Drive's page-moving UI

Previously, users were able to create pages in a separate collection/folder from the workspace, using the ComponentSettingsModal. With this decision, users can only create pages in the collection/folder/resource category they are in. This decision also meant that the only place where the user can move pages is through the MenuDropdown component's Move to tab.

This meant that we no longer have to deal with third nav-related logic for the ComponentSettingsModal, which allows us to rip out a huge chunk of code from the component.

3. New backend endpoints for moving pages

Refer to PR #124 in the backend. This allows us to eliminate complex, multi-step frontend logic (such as the moveFile function in OverviewCard) and replace it with a single API call.

Changes made in this PR

  1. Remove irrelevant code chunks from ComponentSettingsModal. The component was also renamed to ResourcePageSettingsModal since that ended up being the only use case left
  2. Remove unused util functions (saveFileAndRetrieveUrl, as well as the other file name prefix generation util functions)
  3. Replace OverviewCard file-moving logic with a single API call

Changes to make in subsequent PRs

Previously, we allowed pages to be moved using the ComponentSettingsModal.
However, we have since changed our strategy to use a more Google Drive-like
UI, so we won't allow page moving using the ComponentSettingsModal
(now the ResourcePageSettingsModal). Therefore, we no longer have
any need to generate third nav options and collection page data.
Previously, we had a single `saveFileAndRetrieveUrl` function which
handled file-saving and updating behavior across unlinked pages,
collection pages, and resource pages. This made the function extremely
unwieldy and hard to understand.

This commit introduces a separate `saveResourcePage` function which
extracts resource page-specific logic from the original
`saveFileAndRetrieveUrl` function so that it's more readable.
Now that this modal is focused only on Resource pages, there are
large chunks of code in the component which are now irrelevant.

This commit:
- removes all logic related to third navs, since we are now only concerned with resources
  - this allows us to remove creatable select
  - this also allows ut o remove all related handler functions
- remove delete functionality and associated code, since this is handled by the MenuDropdown
- uses the new `saveResourcePage` function to handle file-saving
and updates
Thanks to PR #124 on the backend isomerpages/isomercms-backend#124,
we can simplify the file-moving logic in `OverviewCard` since `OverviewCard` is
now used only for unlinked pages in the workspace (it is also
used for Resource Category display, but these do not have any move file
functionality).

This allows us to remove all the complex logic associated with file-moving
and replace it with a single API call to the new file moving
endpoint. This also allows us to delete the now-unused util function
`saveFileAndRetrieveUrl`.
@kwajiehao
Copy link
Contributor Author

Closing because of overlaps with #388

@kwajiehao kwajiehao closed this Mar 25, 2021
@gweiying gweiying deleted the fix/code-cleanup branch October 13, 2021 03:03
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.

1 participant