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

Resource Registry documentation improvements #518

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

cewing
Copy link
Member

@cewing cewing commented Feb 9, 2016

The documentation of the new Plone 5 resource registry is tough to understand. This PR aims to improve the documentation.

This is a Work In Progress and should not be merged until it has been completed. I am looking specifically for fact-checking and input from @thet, @frapell, @bloodbare, and @ebrehault for Resource Registry concepts and instructions, and from @svx and @polyester for style.

@frisi
Copy link

frisi commented Feb 11, 2016

@cewing did you take a look at #482 (which was my attempt of explaining the process of defining an own bundle)?

please take the time to read my changes and if there is anything that could be taken over to your branch - please do so.
after that, my PR can be closed ;-)

@cewing
Copy link
Member Author

cewing commented Feb 11, 2016

@frisi I'll look that over. In talking to @bloodbare it seems that perhaps it's better for the time being for us not to suggest defining our own bundles to replace the Plone bundle. My impression is that the process is still unsettled and potentially dangerous. Until we have a good story I'd rather leave it out of the docs entirely.

Any opinion on that?

@frisi
Copy link

frisi commented Feb 11, 2016

the example i added is not about overwriting plone and plone-logged-in but create a separate
mysite-plone and mysite-plone-logged-in.

the whole idea was (plip1277 being non-existent) to not end up with 5 different bundles (= 5 requests), just because you installed 3 addons.
addons need to ship a separate bundle to work out of the box w/o compiling resources manually.
even worse: different bundles can possibly contain the same resources (such as jquery, select2 or other stuff they list as required dependencies).

/>

Now we can access the contents within the "static" folder by using the URL part ``++plone++myresources/`` and appending the path to the resource under "static".
Now we can access the contents of the "static" folder in your pakcage by using a URL that starts with ``++plone++myresources/``.
Copy link
Member

Choose a reason for hiding this comment

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

s/pakcage/package

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, will fix.

cewing added a commit that referenced this pull request Feb 15, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ggregation.

Add the merge_with option to the list of available options for bundle registration.
Adjust language to better match the improved and regularized usage in #518
@cewing
Copy link
Member Author

cewing commented Feb 15, 2016

@frisi: having looked at your changes, I am hesitant to put them in place until some of the open questions in them have been answered. If we do have a solid, tested, and functional story for creating a bundle that could be used in place of the plone and plone-logged-in bundles (as I believe your changes would point to), then we should include documentation for how to do that.

However, at this point it seems to me that the story is not solid, has yet to be tested, and may result in some fragility, as site owners would have to re-check their substitute bundles for accuracy and completeness after any upgrade to Plone.

I think the changes in plip1277 probably represent a better, more flexible and safer route forward than suggesting that Plone admins or integrators create their own custom bundles intended to replace plone and plone-logged-in.

Any thoughts on this, @ebrehault or @bloodbare?

@cewing cewing changed the title WIP: Resource Registry doc improvements Resource Registry documentation improvements Feb 15, 2016
@cewing
Copy link
Member Author

cewing commented Feb 15, 2016

@bloodbare, @ebrehault, @svx, @frisi and any other interested parties.

I believe that my work here is ready for review.

I have also updated the plip-1277-meta-bundles-generation branch of this same repository with some language improvements that match better with the changes I've made here. Merging this PR with the changes made there will require a bit of conflict resolution, which I am happy to undertake when the time comes.

Please let me know if there is anything in this updated documentation which requires fixing, or anything I've missed which should be added. I would like to add more example bundle records, for example, to show the use of various of the options available for bundle registration. But I am reasonably satisfied with these changes.

At some point, attention should also be paid to this somewhat parallel documentation. It would be good to reduce duplication by pulling some of the information from one into the other and then using more cross references to tie them together. But that's a discussion for another time, perhaps.

Thanks very much for your time and assistance.

@svx
Copy link
Member

svx commented Feb 15, 2016

There are a couple of .rst related minor issues :)

The most important one is about links, we have that covered in out style-guide :) -> http://docs.plone.org/about/rst-styleguide.html#links

Currently a mix of that and the the .._ reference is used for links in the document, from the point of rst syntax there is nothing wrong with .._ but we do not use that, because it is bad for accessibility [screen-reader for example] and also for translations :).

Line 690: described here is one other issue which we should improve, that is hard for accessibility and SEO, too

Further more I spotted one or two remaining typos, like in line 9

As far is goes for syntax and so on, besides that it looks awesome to me :)

Thanks a lot !

This way we can also support a "development mode" like in Plone versions prior to 5, where changes in the JavaScript sources are instantly reflected (actually they are made available a few seconds after initial browser load).
JavaScript and AMD
------------------

Copy link
Member

Choose a reason for hiding this comment

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

@cewing We should state somewhere that we can write non-AMD JS in an add-on and registrering it in a bundle and that's 100% valid.
AMD is a good practice, and it is provided by default in Plone 5, so that's really easy to use in our context, but it is not mandatory.
A bundle containing something like:

$(document).ready(function() {console.log("Hey, I am not AMD!!!");})

will work fine.
I think it is important to tell it explicitly so people who are afraid of frontend feel more confortable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ebrehault I'd be happy to do so. Can you find an example that does so so that I can point to a clean usage pattern? Right now all the examples I've seen that do not use the AMD pattern are simply defaulting to using jsregistry.xml to get into the plone-legacy bundle

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@ebrehault Perfect! I'll add a mention of this up here at the top, then an example in the bundle registry.

As a question, the JS in that example is dependent on $ being in the window namespace, right? Is that something that is always true? Even under the new resource registry approach? We reliably have a reference to jQuery at $ in the javascript global namespace?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, $ and require are always in the global namespace.

On Tue, Feb 16, 2016 at 10:22 PM, Cris Ewing notifications@github.com
wrote:

In adapt-and-extend/theming/resourceregistry.rst
#518 (comment):

@@ -18,47 +18,75 @@ Introduction to Plone 5 resources

Plone 5 introduces new concepts for working with JavaScript and CSS in Plone.

-We finally have a solution to define JavaScript modules and their dependencies - in contrast to the previous approach, where all dependencies had to be loaded in order.
-We achieve that with the tool RequireJS, which follows the Asynchronous Module Definition (AMD) approach.
-We chose AMD over other module loading implementations (like CommonJS) because AMD can be used in non-compiled form in the browser and also compiled Through-The-Web.
-This way we can also support a "development mode" like in Plone versions prior to 5, where changes in the JavaScript sources are instantly reflected (actually they are made available a few seconds after initial browser load).
+JavaScript and AMD
+------------------
+

@ebrehault https://github.com/ebrehault Perfect! I'll add a mention of
this up here at the top, then an example in the bundle registry.

As a question, the JS in that example is dependent on $ being in the
window namespace, right? Is that something that is always true? Even under
the new resource registry approach? We reliably have a reference to jQuery
at $ in the javascript global namespace?


Reply to this email directly or view it on GitHub
https://github.com/plone/documentation/pull/518/files#r53080616.

@cewing
Copy link
Member Author

cewing commented Feb 15, 2016

@svx I think I've now addressed your comments. I've unified all external links using the recommended pattern. I've fixed the typo on line 9 and I've changed up the link text on what is now line 722 to make it more clear where it will take you.

I do want to include something about the newly-added option for bundles (stub_js_modules), but it's not yet released, right? Do we have a way of adding text to documentation that is hidden so that we can un-hide it when that feature is released publicly? Alternatively, could I mark a give feature as "added in Plone 5.x.y" so that it is clear that it isn't available before that time?

@svx
Copy link
Member

svx commented Feb 16, 2016

@cewing no currently there is no way to hide stuff as in text, you can 'hide'stuff from/in the toctree but that is currently all.

As for versions there is http://www.sphinx-doc.org/en/stable/markup/para.html?highlight=version#directive-versionadded quite handy, and I would like to see it more often used, if we change/add things in minor versions, I guess that is what you asking for ?

Thanks btw, for all your work on it ! :)

@cewing
Copy link
Member Author

cewing commented Feb 16, 2016

@svx, that directive is exactly what I'm looking for. All I need to know now is which version of Products.CMFPlone will contain the new stub_js_modules. Then I can add that documentation in anticipation of it coming out. @esteele can you comment on that? The feature in question is on trunk now in Products/CMFPlone/interfaces/resources. Do you have an idea which future Plone release will contain it?

This allows a "development mode" where changes in JavaScript source files are reflected in the browser in near real-time.

It is also possible in the new Resource Registry to provide bundles with simple Javascript that does not make use of AMD.
See :ref:`bundle_example_non_amd` below for an example of such a bundle.
Copy link
Member Author

Choose a reason for hiding this comment

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

@ebrehault this paragraph is added to address your comment above.

@seanupton
Copy link
Member

@cewing @svx reading changelogs, seems like stub_js_modules support was released in Products.CMFPlone 5.0.1, so should be in current (but not yet advertised as current on Plone.org downloads) Plone 5.0.2. If so, I would hope it safe to document now without disclaimer?


Example based on Plone's standard bundles defined in ``Products/CMFPlone/profiles/dependencies/registry.xml``
In development mode, each bundle loads all of its resources in the rendered site as individually, with individual requests.
Copy link
Member

Choose a reason for hiding this comment

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

... as individually, with individual requests maybe my english is not sufficient, but i dont get what that means exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a typo, @jensens. The sentence should be this:

"In development mode each bundle loads all its resources individually."

Thanks for catching it. I'll fix it asap.

@jensens
Copy link
Member

jensens commented Feb 16, 2016

Great work and a major improvement!

@svx
Copy link
Member

svx commented Feb 18, 2016

@cewing @jensens do guys had time to have a look at #518 (comment) yet ?

I really would like to see that merged soon, since it is huge improvement to the docs !

Thanks again, to all ppl who took some time to work/review/help to improve it !

@cewing
Copy link
Member Author

cewing commented Feb 18, 2016

@svx, I've fixed the problems @jensens noted, and adjusted the wording for the plone-compile-resources script to clarify that you need a stopped instance. I think that's non-controversial and should not hold up merging.

I've also added documentation for the stub_js_modules option for a bundle record and noted that it was added in 5.0.1, so we should be good to go on merging.

thanks, all

@svx
Copy link
Member

svx commented Feb 20, 2016

Just to make sure one last time, everyone is fine with merging ?

I ask because this part of Plone is really not my part of wisdom, so to say

@cewing
Copy link
Member Author

cewing commented Feb 20, 2016

I believe we are ready.

@jensens
Copy link
Member

jensens commented Feb 21, 2016

i reviewed it and its so much much better now, please merge and release soon :)

@gforcada
Copy link
Member

Can we get some commits squashed/fixup'ed before? :-) https://docs.plone.org/develop/coredev/docs/git.html

ebrehault pushed a commit that referenced this pull request Feb 22, 2016
…ggregation.

Add the merge_with option to the list of available options for bundle registration.
Adjust language to better match the improved and regularized usage in #518
@svx
Copy link
Member

svx commented Feb 23, 2016

@cewing do you want to squash ? :)

@cewing
Copy link
Member Author

cewing commented Feb 24, 2016

@svx, will do. Gimme a few minutes here :)

Atomic commit messages to follow:

update the introduction to improve the structure, clarify a few explanations, add links to external definitions of terms, and provide a bit more background for the concepts to follow

small improvement to wording

improve the clarity of the description of bundle registration.  Be more explicit about what is allowed, required, etc

improve the documentation about various options for resource registry records, include cross-links for better clarity

provide more numerous examples of different forms of resource records.  Include more textual explanation of the important attributes of each example

add a note about the difference between values for the js option and those for the css option

minor grammatical fixes

minor improvements to docs regarding resources

begin re-organizing bundle section of the documentation.  Make formatting and language around defining and configuring bundles consistent with the language used for resources.  Make section sub-headings more consistent

fix method signature to make it clear this is for resources, not bundles

improve the section on compiling bundles, including specific instructions on making the compiling script available.  Fix up the organization a bit to better match the resources section

improve documentation of the new plone traversal namespace

improve explanation of using your own build system for compiled bundle resources

improve structure and English syntax of the description of the mismatched anonymous define() error

fix errors and omissions noted by @seanupton

improvement to naming.  The 'static' part should not be included in the title, as it is specific to Plone's own registration of a folder for that namespace.

Update the section on Diazo Theme Bundles
Provide improved Bundle registration examples (more are needed)
Improve documentation of the resource request APIs
Fix up the build to migrating older code, including links to more complete documentation of module definition patterns.
Improve the syntax of the English in the section on including non-RequireJS resources in Diazo themes.

add a few more example bundles thanks to @ebrehault.  Also improve the descriptions of each to point out some differences in usage and the reasons for them

fix typo, thx @svx

fix external linking style according to styleguide.  Thx @svx for the pointer.

improve the link text a bit for this link in accordance with @svx's wishes.

follow the lead of @svx in #522 and spell it 'Javascript'

provide an example of registering a simple non-AMD javascript as a 'pre-compiled' bundle (thanks @ebrehault)

Revert "follow the lead of @svx in #522 and spell it 'Javascript'"

I misread the title of that commit and did exactly the wrong thing.

This reverts commit 09f9e58.

fix wording for clarity, thx @jensens

document the stub_js_resources option and mention it's use in the documentation for minimizing bundles.  Replace the docs for adding your stuff to the plone bundle as that is not recommended
@cewing cewing force-pushed the resource_registry_doc_improvements branch from 2ecd37d to 3dbff0c Compare February 24, 2016 18:30
@cewing
Copy link
Member Author

cewing commented Feb 24, 2016

@svx, this is squashed and ready to merge. Fire away!

svx pushed a commit that referenced this pull request Feb 24, 2016
Resource Registry documentation improvements
@svx svx merged commit 9bcb026 into 5.0 Feb 24, 2016
@svx svx deleted the resource_registry_doc_improvements branch February 24, 2016 18:45
@svx
Copy link
Member

svx commented Feb 24, 2016

Yes ! Thanks ! Beer in Boston !

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

Successfully merging this pull request may close these issues.

None yet

7 participants