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

Use all the apiExpanders in use, so we perform a single request for getting all the required data. #4946

Merged
merged 14 commits into from
Jul 18, 2023

Conversation

sneridagh
Copy link
Member

No description provided.

@netlify
Copy link

netlify bot commented Jul 5, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 36ef2e7
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64b698db46f0cd000802c171
😎 Deploy Preview https://deploy-preview-4946--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tiberiuichim
Copy link
Contributor

I don't know if there's an extra check for duplicate appextras, some projects may have added themselves. A word in the upgrade guide is needed, for sure.

@@ -0,0 +1 @@
Use all the apiExpanders in use, so we perform a single request for getting all the required data. @sneridagh
Copy link
Contributor

Choose a reason for hiding this comment

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

I am no Steve, but I will still suggest a minor change just to avoid the repetition of the word "use" and to clarify 😛

Suggested change
Use all the apiExpanders in use, so we perform a single request for getting all the required data. @sneridagh
Use all the available apiExpanders, so we perform a single request for getting all the required data. @sneridagh

@@ -76,6 +76,8 @@ let config = {
publicURL,
apiPath,
apiExpanders: [
// Added here for documentation purposes, addded at the end because it
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Added here for documentation purposes, addded at the end because it
// Added here for documentation purposes, added at the end because it

match: '',
GET_CONTENT: ['navigation'],
querystring: {
'expand.navigation.depth': config.settings.navDepth,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work if final projects change navDepth? I am not entirely sure, or I don't get why it works at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct, it won't... since it's passed once, with the "old", default value... Not an easy one :/

Suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

@sneridagh An idea: make it possible to pass a function that constructs the querystring, instead of a prebuilt querystring object.

Copy link
Member Author

@sneridagh sneridagh Jul 7, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it. Now that this is dealt with, I would move this whole section to line 78, where apiExpanders is set inside config.settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pnicolli it turns out we can't because we haven't set config.settings.navDepth, unless we assume we are going to sync them manually (I guess that the default is not going to change that much).

@cypress
Copy link

cypress bot commented Jul 7, 2023

Passing run #6443 ↗︎

0 529 20 0 Flakiness 0

Details:

Merge branch 'master' into useapiexpandersincore
Project: Volto Commit: 36ef2e7fbb
Status: Passed Duration: 13:42 💡
Started: Jul 18, 2023 1:54 PM Ended: Jul 18, 2023 2:08 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@sneridagh
Copy link
Member Author

@davisagli for some reason the Plone 5 expander for types is not working? :/ How that could be?

… of the token, in order to remove the types expander safely
@sneridagh sneridagh added this to the 17.x.x milestone Jul 17, 2023
@sneridagh sneridagh requested a review from pnicolli July 17, 2023 18:18
@sneridagh
Copy link
Member Author

@davisagli @pnicolli ready.

@sneridagh sneridagh modified the milestones: 17.x.x, Plone 6.1 Jul 18, 2023
@sneridagh sneridagh merged commit 8c53970 into master Jul 18, 2023
39 checks passed
@sneridagh sneridagh deleted the useapiexpandersincore branch July 18, 2023 14:13
sneridagh added a commit that referenced this pull request Jul 24, 2023
* master: (42 commits)
  make selectedView and className props available for Search block (#4997)
  Release @plone/volto-testing 4.0.0-alpha.0
  Release 17.0.0-alpha.21
  Upgrade to Cypress 12.17.1 (latest) (#4981)
  Image rendering (#3337)
  feat(Url.js): add getFieldURL helper function to get the url value of a field based on its structure (#4731)
  Handle @linkintegrity response with items but no breaches (#4832)
  Release 17.0.0-alpha.20
  Use all the apiExpanders in use, so we perform a single request for getting all the required data. (#4946)
  Fix the condition deciding on listing pagination format so it takes into account container blocks as well (#4978)
  Release 17.0.0-alpha.19
  Fix search block input clear button doesn't reset the search (#4837)
  Add /ok route as an express middleware (#4432)
  handles condition for yearly frequency in recurrence (#4604)
  Remove dangling out of place Guillotina Cypress tests (#4980)
  Update to latest plone.restapi and Plone 6.0.6 (#4979)
  Update browserlist (#4977)
  `Links and references` view via content menu [Add `Links to item` view] (#4787)
  Release 17.0.0-alpha.18
  Toc responsive (#4912)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants