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

Update assembly-pantheon-help.adoc #504

Closed
wants to merge 1 commit into from

Conversation

pwright
Copy link

@pwright pwright commented Jan 26, 2021

@pwright
Copy link
Author

pwright commented Jan 26, 2021

If this PR is merged, you can view the whole guide (using the chrome extension above) at https://raw.githubusercontent.com/pwright/pantheon/patch-1/pantheon-bundle/src/main/resources/SLING-INF/content/docs/assemblies/assembly-pantheon-help.adoc

@stoobie
Copy link
Contributor

stoobie commented Mar 9, 2021

Hey @pwright Thanks for the contribution!

If I understand correctly, the goal in this PR is to enable previewing the Pantheon 2 documentation using a Google Chrome extension. I'm guessing that the extension doesn't support the use of symlinks in file paths, which would explain why you changed the include file path. Is that correct?

So first of all, be aware that because of Pantheon 2's current lack of support for nested assemblies, we are about to merge (PR 533)[https://github.com//pull/533] which will severely impact this PR (504). You might want to take a look at that one.

Secondly, it would be helpful (if you decide to update this PR after we merge 533) to include some text describing asciidoctorjs-live-preview and where to find info on installing and using it. I think that might actually make the most sense to appear in the README.

Thirdly, be aware that the EUD is slated to be released and viewable from within Pantheon in the next release of Pantheon, which I am given to understand will be within a few weeks. Once it is viewable from within Pantheon, do you still see benefit to this PR?

@pwright
Copy link
Author

pwright commented Mar 9, 2021

My reading of the related PR (533) is that we're going 'All in' wrt symlinks.
If true, feel free to close this PR as it relies on not using symlinks.

viewable in pantheon requires vpn access, and will only show latest published version. This PR would allow viewing of all branches, versions, forks, etc (that's the point of it)

@pwright
Copy link
Author

pwright commented Mar 10, 2021

@stoobie You made some good points above, I'll follow up with #541

@pwright pwright marked this pull request as draft March 10, 2021 10:37
@stoobie
Copy link
Contributor

stoobie commented Mar 10, 2021

@stoobie You made some good points above, I'll follow up with #541

Great, @pwright , looks good. I'm looking forward to trying it out! It sounds like a decent solution.

@stoobie
Copy link
Contributor

stoobie commented Mar 17, 2021

@pwright So now that #533 is merged, and you opened #541, is this current PR still necessary?

@pwright
Copy link
Author

pwright commented Mar 18, 2021

@stoobie you're correct, closing

@pwright pwright closed this Mar 18, 2021
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.

2 participants