Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Support for Knot.x setup in gradle aem multi #65

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

mierzwid
Copy link
Contributor

@mierzwid mierzwid commented Jun 7, 2019

No description provided.

@pun-ky pun-ky changed the base branch from master to develop June 7, 2019 13:58
@tomaszmichalak
Copy link

Can you please check https://github.com/Knotx/knotx-starter-kit. It can be very useful when you consider Knot.x extensions development.

@malaskowski malaskowski changed the title Feature/support for knotx 61 Support for Knot.x setup in gradle aem multi Jun 14, 2019
Copy link
Contributor Author

@mierzwid mierzwid left a comment

Choose a reason for hiding this comment

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

@tomaszmichalak all looks good, only one question to the code.

More general comment:
Please add section in README.md describing this Knot.X integration (with links to Knot.X documentation) - how it works and which endpoint contains page with data injected via knotx.
Without docs this whole feature is somewhat hidden.

@@ -27,6 +29,7 @@ configure<AemExtension> {
url("Demo site", "http://demo.example.com/en-us.html", text = "English")
url("Author login", "http://author.example.com/libs/granite/core/content/login.html" +
"?resource=%2F&\$\$login\$\$=%24%24login%24%24&j_reason=unknown&j_reason_code=unknown", text = "AEM Sign In")
url("Knot.x", "http://knotx.example.com/products/details.html", text = "Hello Knot.x with GAP")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<3

Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike comments / code looking like a poc. I more like looking like as much as production as it is possible

@@ -56,10 +56,11 @@ class PageModel : Serializable {
}

private fun determineLanguageCode(): String {
val languagePath = LanguageUtil.getLanguageRoot(resource.path)
val languagePart = languagePath.substringAfterLast("/")
// val languagePath = LanguageUtil.getLanguageRoot(resource.path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why those lines are commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably code need to be only hardened

Copy link
Contributor

@pun-ky pun-ky left a comment

Choose a reason for hiding this comment

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

Thx guys, this PR now looks nice 😊 @Skejven the domain related problem is solved?

@tomaszmichalak
Copy link

Thx guys, this PR now looks nice 😊 @Skejven the domain related problem is solved?

yes, it is solved in RC5 that we released today ;)

@pun-ky
Copy link
Contributor

pun-ky commented Jul 24, 2019

after catch up with @tomaszmichalak agreeded TODO looks as below:

  1. cover both demo and live domains by knotx (best if it will use same / shared parts of configuration)
  2. improve / make nicer products demo page / rendered products with images based on bootstrap 4 card markup and json from e.g https://jsonplaceholder.typicode.com/photos / imitate real products by some mock JSON API ; knotx part will use to https://github.com/Knotx/knotx-data-bridge/tree/master/http to call this API
  3. inverse domain logic:
  • example.com <=> knotx.example.com / dispatcher.example.com (example.com should be most rich / complete response / looking as production one)
  • demo.example.com <=> knotx.demo.example.com / dispatcher.demo.example.com

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants