-
Notifications
You must be signed in to change notification settings - Fork 275
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
[templates/nextjs] [sitecore-jss-nextjs] [sitecore-jss] Remove partial rendering implementation (EditingComponentPlaceholder component) #1821
Conversation
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.
Looks good! See some comments below, mostly related to documentation
CHANGELOG.md
Outdated
@@ -24,7 +24,7 @@ Our versioning strategy is as follows: | |||
|
|||
### 🛠 Breaking Change | |||
|
|||
* Editing Integration Support: ([#1776](https://github.com/Sitecore/jss/pull/1776))([#1792](https://github.com/Sitecore/jss/pull/1792))([#1773](https://github.com/Sitecore/jss/pull/1773))([#1797](https://github.com/Sitecore/jss/pull/1797))([#1800](https://github.com/Sitecore/jss/pull/1800))([#1803](https://github.com/Sitecore/jss/pull/1803))([#1806](https://github.com/Sitecore/jss/pull/1806))([#1809](https://github.com/Sitecore/jss/pull/1809))([#1814](https://github.com/Sitecore/jss/pull/1814))([#1816](https://github.com/Sitecore/jss/pull/1816))([#1819](https://github.com/Sitecore/jss/pull/1819)) | |||
* Editing Integration Support: ([#1776](https://github.com/Sitecore/jss/pull/1776))([#1792](https://github.com/Sitecore/jss/pull/1792))([#1773](https://github.com/Sitecore/jss/pull/1773))([#1797](https://github.com/Sitecore/jss/pull/1797))([#1800](https://github.com/Sitecore/jss/pull/1800))([#1803](https://github.com/Sitecore/jss/pull/1803))([#1806](https://github.com/Sitecore/jss/pull/1806))([#1809](https://github.com/Sitecore/jss/pull/1809))([#1814](https://github.com/Sitecore/jss/pull/1814))([#1816](https://github.com/Sitecore/jss/pull/1816))([#1819](https://github.com/Sitecore/jss/pull/1819)[#1821](https://github.com/Sitecore/jss/pull/1821)) |
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.
This change is not related to Editing Integration, should be added just as a separate bullet point
docs/upgrades/unreleased.md
Outdated
Sitecore Pages supports component rendering to avoid refreshing the entire page during component editing. | ||
If you are using Experience Editor only, this logic can be removed, Layout can be left. | ||
*/} | ||
{isComponentRendering ? ( |
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.
You can just provide this part as an example (including imports) that should be removed from the app, not sure if we need to copy/paste all the file content here. Like we are doing for other upgrade steps
CHANGELOG.md
Outdated
@@ -36,6 +36,7 @@ Our versioning strategy is as follows: | |||
* `[sitecore-jss]` Introduced `GraphQLEditingService` class to fetch editing data in Metadata Edit Mode. | |||
* `[templates/nextjs-xmcloud]` Introduced _/lib/graphql-editing-service_ to fetch editing data in Metadata Edit Mode. | |||
* `[templates/nextjs-xmcloud]` Added a new _page-props-factory/plugins/preview-mode_ plugin to handle both Chromes and Metadata Edit Mode. | |||
* `[templates/nextjs]` `[sitecore-jss-nextjs]` `[sitecore-jss]` Remove EditingComponentPlaceholder as it will not be used by Pages in its current implementation |
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.
Can you add more details here, please?
Mention removed types. variables that could be used by Pages (e.g. RenderingType)
docs/upgrades/unreleased.md
Outdated
@@ -134,4 +134,25 @@ | |||
... | |||
} | |||
... | |||
``` | |||
|
|||
* The implementation of 'EditingComponentPlaceholder' has been removed. Its purpose to avoid refreshing the entire page during component editing in Pages had never been fully utilized. The references to it and to `RenderingType` enum in `[[...path]].tsx` of the nextjs app (and in any custom code) should be removed: |
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.
Not related to nextjs-xmcloud, should be mentioned under nextjs section
@yavorsk Let's update the title a bit a be more specific on the feature, e.g. "[templates/nextjs] [sitecore-jss-nextjs] [sitecore-jss] Remove Partial Rendering implementation" |
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.
Looks good! 👍
Description / Motivation
This PR removes the EditingComponentPlaceholder implementation as it will not be used by Pages in this form:
consists of:
Testing Details
Types of changes