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/update pageroutes #133

Merged
merged 18 commits into from
Mar 17, 2021
Merged

Feat/update pageroutes #133

merged 18 commits into from
Mar 17, 2021

Conversation

gweiying
Copy link
Contributor

@gweiying gweiying commented Mar 16, 2021

This PR updates the pages and collectionPages read, update, delete and rename routes so that all path names are received and parsed as encodedURIComponents, and Base64 encoding happens only on the backend. This PR extends the changes introduced by Alex in #131.

For collectionPages delete and rename, the collection config is also updated accordingly to remove/ rename the modified pages.

This PR also updates the CollectionConfig's addItemToOrder, deleteItemFromOrder, updateItemInOrder and read methods to make the class more reusable.

kwajiehao and others added 8 commits March 3, 2021 14:21
* feat: add RootType Directory

This commit adds a RootType to the Directory class so that we can
retrieve directories from the root of a repo.

* feat: new endpoint for retrieving information on all folders

This commit introduces a new GET `/:siteName/folders/all` endpoint
which:
1) gets the names of all collections/folders within the repo
2) gets the collection.yml directory config file for each collection

* fix: remove _ before sending folder name

Co-authored-by: jiehao <[email protected]>
@gweiying gweiying changed the base branch from Feat-read-from-nested-files-and-update-page-endpoints-for-nested-files to staging March 16, 2021 02:13
@gweiying gweiying force-pushed the feat/update-pageroutes branch from 02f1a80 to e1c378d Compare March 16, 2021 02:18
@gweiying gweiying marked this pull request as ready for review March 16, 2021 02:20
@gweiying gweiying requested review from alexanderleegs and kwajiehao and removed request for alexanderleegs March 16, 2021 02:20
Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

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

looks good for the most part, mostly had a question on how the updateItemInOrder method for the CollectionConfig class works

classes/Config.js Show resolved Hide resolved
return { index, item }
}

async updateItemInOrder(oldItem, newItem) {
Copy link
Contributor

@kwajiehao kwajiehao Mar 17, 2021

Choose a reason for hiding this comment

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

It's nice to compose the updateItemInOrder from the other methods, but this results in two four API calls for what is essentially one update. Have we considered writing specific "update" code so only one two API calls are made?

Copy link
Contributor

@kwajiehao kwajiehao Mar 17, 2021

Choose a reason for hiding this comment

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

@gweiying brought up the possibility of building out the methods for the CollectionConfig class but as that would be a more significant undertaking we will be postponing that for now. As discussed, we will modify the updateItemInOrder method so that the the directory config file is read only once, and custom updating logic is applied instead of composing it with the addItem and deleteItem methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -63,24 +63,6 @@ async function listPages (req, res, next) {
res.status(200).json({ pages })
}

// Create new page
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could we also remove unused imports for this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

lgtm!

As discussed offline, we might want to move the encoding/decoding into File instead, but we'll do this later on when cleaning up

Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

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

lgtm

@gweiying gweiying merged commit 50aa6e5 into staging Mar 17, 2021
@gweiying gweiying deleted the feat/update-pageroutes branch March 17, 2021 10:29
harishv7 pushed a commit that referenced this pull request Feb 17, 2023
* Feat/get all collections directories (#117)

* feat: add RootType Directory

This commit adds a RootType to the Directory class so that we can
retrieve directories from the root of a repo.

* feat: new endpoint for retrieving information on all folders

This commit introduces a new GET `/:siteName/folders/all` endpoint
which:
1) gets the names of all collections/folders within the repo
2) gets the collection.yml directory config file for each collection

* fix: remove _ before sending folder name

Co-authored-by: jiehao <[email protected]>

* fix: resolving merge conflicts

* fix: restores comments that were accidentally deleted

* fix: applies missing URI decoding to page routes

* fix: updates CollectionConfig to return and accept spliced index

* fix: updates renameCollectionPage to use updated CollectionConfig functions

* fix: adds updateItemInOrder and read methods to CollectionConfig

* fix: updates collectionPages route to use updated CollectionConfig methods

* fix: changes updateItemInOrder to use single read and update API call

* fix: removes unused imports

Co-authored-by: kwajiehao <[email protected]>
Co-authored-by: jiehao <[email protected]>
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.

3 participants