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

Issues/618 htl server #624

Merged
merged 57 commits into from
Apr 26, 2021
Merged

Issues/618 htl server #624

merged 57 commits into from
Apr 26, 2021

Conversation

cmrockwell
Copy link
Collaborator

@cmrockwell cmrockwell commented Oct 22, 2020

Notable changes presented by this PR

  • added Maven module pagerender/server to parent pom.xml. This will allow the server side HTL module to compile as a reactor module. Without a theme installed, users cannot create new sites. It allows the work to be safely merged to develop-sling12 without impacting rendering styles
  • Updated PageMerge.java adding a new method that will return a list of merged resources for htl rendering
  • Updated pagerender/server pom.xml such that the project builds successfully
  • Updated pagerender/server example/test content
  • Added 3 Content Merge Scenarios to AdaptionJTest
  • Updated Container.java under pagerender/server project such as that container content merges properly
  • Refactored the Sling Junit Tests such that they are separated by runmodes
  • Allows per:Site to have site settings
  • Introduces a concept of components allowed by templates (this change is coming in a separate pr)

This PR relates to issues #618 #624 #621 #620

…h common-lang dependency. Also frontend-maven-plugin (node) was failing, but not sure what it does for server/ui.apps. Commenting out for now, also the module to build, install and deploy to Sling
Fix NPE due to incorrect the init method local variable 'resource' overriding instance property
…mple vue site can have new page without errors. Added TODO comments about nav and footer for these sites
…nderserver/components. Adjusted PageRenderServerConstants.java and /content/pagerenderserver/ paths accordingly
@cmrockwell cmrockwell added the htl Sling HTL Rendering label Oct 22, 2020
@cmrockwell cmrockwell requested a review from reusr1 October 22, 2020 17:23
…tion (AdaptTo) for per:Page and per:Asset types; and their ability to get the correct per:Site resource as implemented in PerBaseImpl.java
…. Refactored Helper.java to allow siteCSS to exist under per:Site/jcr:content
…e pagerenderer/server module, it may be useful to continue support the npm build processes. this commit updates a few deps to keep updated with the other modules. may be necessary to mvn clean -PcleanNodeModules or reinstall via npm i.
…rning a list of resources coming either from the template or the page content.
…elpers alike. Refactored PageHelper Use class to extend BaseHelper
…ed PageHelper to use getters instead of direct access
@cmrockwell
Copy link
Collaborator Author

Please merge this part to develop-sling12. I plan to move attention to the authoring side tickets for SSR. This PR adds solutions to the bullet points listed in the description. I don't like building for weeks & months on one branch only to face nasty merge conflicts in the end.

@reusr1
Copy link
Contributor

reusr1 commented Nov 2, 2020

@cmrockwell since you are changing the page merger here - does this have any impact on the overall vue js renderer? Can you explain what is changing in the data.json output due to the change of the merger?

@cmrockwell
Copy link
Collaborator Author

cmrockwell commented Nov 2, 2020

There's no impact to Vue JS (data.json) rendering. The changes I'm making to PageMerge.java are to support a new method getMergedResources The new method uses the same map used for Vue rendering, and the method returns a flattened ordered List of Sling Resource for HTL rendering

@reusr1 reusr1 marked this pull request as draft November 3, 2020 15:37
…riptHelper is null, then it will try to get the modelFactory from the bindings, which is added by the Adaption Jtest adds it. Use classes have bindings such as resource. Getting the resource from the request was problematic for testing, it is easier to test if PageMaerg.java uses the resource reference from the bindings, and not the reference attached to the request
…d to use resource from the bindings and not a reference through the request
…st whether getMergedResources properly merges template content with page content for HTL rendering
…the serverSide property. if it exists and is true, then load the page. Otherwise proceed with clientside rendering.
@cmrockwell
Copy link
Collaborator Author

Based on the discussion in the water cooler meeting, I reviewed and tested the changes again.
I stand by the changes. I think there is limited exposure to vue stuff, but I have tested that and don't there are any new bugs. Some of the changes were because Ruben asked to move stuff from pagerenderserver/structure to pagerenderserver/components. Or asked to have junit tests under different runmodes. The XML content is there for testing. But now after hearing the team's concerns, I'm doubting if this is better left on a fork.

@reusr1
Copy link
Contributor

reusr1 commented Apr 14, 2021

@cmrockwell I think this is a very valuable addition to peregrine and should be merged and in my opinion it should not be kept on a fork.

@reusr1
Copy link
Contributor

reusr1 commented Apr 19, 2021

@cmrockwell was testing peregrine a bit with the server side rendered PR in it - looks nice :-) got a couple of questions:

  • I am trying to understand the /apps/nt/unstructured addition to understand how it is used and if it has a side effect
  • copy/paste of components seems to not refresh the page at the moment

I also took the liberty to

  • change the elements in the pom's to be in line with the vue renderer: 3cf2125
  • added server side rendering to the docker image build as a supported renderer out of the box

@cmrockwell
Copy link
Collaborator Author

I am trying to understand the /apps/nt/unstructured addition to understand how it is used and if it has a side effect

Here's a refresher about the /apps/nt/unstructured addition. It helps render the container that came from template, to which an editor added components.

This page is an example
http://localhost:8080/content/pagerenderserver/pages/non-empty-container.html

It uses template
http://localhost:8080/bin/browser.html/content/pagerenderserver/templates/empty-container

This template has the container but no other content
http://localhost:8080/bin/browser.html/content/pagerenderserver/templates/empty-container/_jcr_content/content

When an editor creates and edits a page using this template (like a text component)
http://localhost:8080/bin/browser.html/content/pagerenderserver/pages/non-empty-container/_jcr_content/content/text1

Then the intermediate paths for the container become nt:unstructured within the page's content
http://localhost:8080/bin/browser.html/content/pagerenderserver/pages/non-empty-container/_jcr_content/content
jcr:primaryType = "nt:unstructured"

And since there is currently no html rendering for nt:unstructured, we decided to implement that instead of adding resourceTypes for the intermediate containers.

@cmrockwell cmrockwell requested a review from flx-sta April 20, 2021 14:57
Copy link
Contributor

@reusr1 reusr1 left a comment

Choose a reason for hiding this comment

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

approving this - we can deal with issues that may come up because of this PR later

Copy link
Contributor

@flx-sta flx-sta left a comment

Choose a reason for hiding this comment

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

Some small changes requested 🚀

…data property in peregrineApp.js, remove commented and unused lines from NavModel.java, use the common pattern for public static final String"

"
@cmrockwell cmrockwell requested a review from flx-sta April 24, 2021 19:11
// } else {
processLoadedContent(response.data, path, firstTime, fromPopState)
// }
if (response.hasOwnProperty('data') && response.data.serverSide === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
could have just used response.data && ... but doesn't really matter

@cmrockwell cmrockwell merged commit f5bce05 into develop-sling12 Apr 26, 2021
@cmrockwell cmrockwell deleted the issues/618-htl-server branch April 26, 2021 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
htl Sling HTL Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants