-
Notifications
You must be signed in to change notification settings - Fork 9
feat(Ruby): add intro sentence to summary with friendly api name #373
Conversation
…gleapis/doc-templates into title-with-friendly-api-name
…gleapis/doc-templates into title-with-friendly-api-name
…gleapis/doc-templates into title-with-friendly-api-name
* factor out setting property type * factor out property type name from ManagedReference pre-renderer * remove dependency on partials/title * modify to access first element in name * edit title in common.js based on type * chore: write title in preprocessor instead of partial template * fix minor bugs, add new line on </h1> to match golden * fix: use model type instead of javaType, loosen title restriction in template * fix: remove newline in <h1>, document getTitleForTypeProperty function Co-authored-by: Dan Lee <[email protected]>
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [google-cloud-storage](https://togithub.com/googleapis/python-storage) | `~=2.2.0` -> `~=2.3.0` | [![age](https://badges.renovateapi.com/packages/pypi/google-cloud-storage/2.3.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/google-cloud-storage/2.3.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/google-cloud-storage/2.3.0/compatibility-slim/2.2.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/google-cloud-storage/2.3.0/confidence-slim/2.2.1)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/python-storage</summary> ### [`v2.3.0`](https://togithub.com/googleapis/python-storage/blob/HEAD/CHANGELOG.md#​230-httpsgithubcomgoogleapispython-storagecomparev221v230-2022-04-12) [Compare Source](https://togithub.com/googleapis/python-storage/compare/v2.2.1...v2.3.0) ##### Features - add dual region bucket support and sample ([#​748](https://togithub.com/googleapis/python-storage/issues/748)) ([752e8ab](https://togithub.com/googleapis/python-storage/commit/752e8ab42d23afd68738e4d7ca6cdeee416dfd50)) - track invocation id for retry metrics ([#​741](https://togithub.com/googleapis/python-storage/issues/741)) ([bd56931](https://togithub.com/googleapis/python-storage/commit/bd5693164e7331df5f14186fd002e72e5203d7ee)) ##### Bug Fixes - **deps:** drop pkg_resources ([#​744](https://togithub.com/googleapis/python-storage/issues/744)) ([e963f33](https://togithub.com/googleapis/python-storage/commit/e963f33ced2852b64d721d69928b54443461ec9c)) ##### Documentation - fix links in blob module ([#​759](https://togithub.com/googleapis/python-storage/issues/759)) ([9b29314](https://togithub.com/googleapis/python-storage/commit/9b2931430b0796ffb23ec4efacd82dacad36f40f)) ##### [2.2.1](https://togithub.com/googleapis/python-storage/compare/v2.2.0...v2.2.1) (2022-03-15) ##### Bug Fixes - remove py.typed marker file for PEP 561 ([#​735](https://togithub.com/googleapis/python-storage/issues/735)) ([f77d2f7](https://togithub.com/googleapis/python-storage/commit/f77d2f787f435f2f898e9babcdab81225672ad4f)), closes [#​734](https://togithub.com/googleapis/python-storage/issues/734) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/doc-templates).
This adds a friendly intro sentence to the page's summary. This sentence is only generated if the yaml includes a "friendlyApiName" key.
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.
Will you merge #367, git pull on your branch and then push only with the relevant commits?
@@ -102,7 +105,7 @@ function handleItem(vm, gitContribute, gitUrlPattern) { | |||
vm.sourceurl = common.getViewSourceHref(vm, null, gitUrlPattern); | |||
|
|||
// set to null incase mustache looks up | |||
vm.summary = vm.summary || null; | |||
vm.summary = vm.summary != null ? vm.summary : null; |
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.
How are the two different?
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 allows us to modify a summary that is an empty string. Without the distinction, we won't be able to add this sentence for pages contain an empty summary, but do have a summary
key in the yaml.
third_party/docfx/templates/devsite/UniversalReference.common.js
Outdated
Show resolved
Hide resolved
* @param {string} friendlyApiName The friendly API title for the item. | ||
* @param {Array} name The names for the item, only the first item is used for forming the sentence. | ||
*/ | ||
function addIntroSentenceToSummary(summary, friendlyApiName, type, name) { |
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 looks fairly complicated compared to adding it to the mustache template. How about doing this in the template instead with something like...
{{#summary}}
{{#friendlyApiName}} Reference documentation and code samples for the {{friendlyApiName}} {{title}}.{{/friendlyApiName}}
{{summary}}
{{/summary}
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.
It gets complicated when including the object's type in the sentence. I factored out the type addition to title so we wouldn't have to have a partial file for that. This would be similar: we'd need to have a partial for switching based on type.
In general, I think we should try to avoid hard-coding language in the template -- it makes things like language updates and localization efforts more difficult.
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.
We won't be doing localization based on the template given here. That's fine.
Another consideration is to add this for now into the Ruby YAMLs to begin with, which would be easier to find all the values rather than trying to compute and figure out what we need to extract from the YAMLs.
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.
Discussed offline, in summary:
There are trade-offs between at generator level or template level. There may be more complexity to implement this at the language-generator level, but it'd also simplify our template. We can revisit this approach if we expand this update to other languages.
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 sure I understand. Feels funny to be dealing with <p>
tags here when we have the HTML template, which should handle... the HTML? If we want to come up with the content here to avoid the type complexity, we should at least leave off the <p>
and </p>
and add those in the template.
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.
🤦🏻♂️, right that'd be a good idea! I put the <p>
tags in the template, let me know what you think.
Co-authored-by: Dan Lee <[email protected]>
* @param {string} friendlyApiName The friendly API title for the item. | ||
* @param {Array} name The names for the item, only the first item is used for forming the sentence. | ||
*/ | ||
function addIntroSentenceToSummary(summary, friendlyApiName, type, name) { |
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.
We won't be doing localization based on the template given here. That's fine.
Another consideration is to add this for now into the Ruby YAMLs to begin with, which would be easier to find all the values rather than trying to compute and figure out what we need to extract from the YAMLs.
Co-authored-by: Dan Lee <[email protected]>
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.
Could you update this with updated Ruby YAML with the friendly name field to see how this looks in practice?
* @param {string} friendlyApiName The friendly API title for the item. | ||
* @param {Array} name The names for the item, only the first item is used for forming the sentence. | ||
*/ | ||
function addIntroSentenceToSummary(summary, friendlyApiName, type, name) { |
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 sure I understand. Feels funny to be dealing with <p>
tags here when we have the HTML template, which should handle... the HTML? If we want to come up with the content here to avoid the type complexity, we should at least leave off the <p>
and </p>
and add those in the template.
Merged #367 and current branches reflects changes.
This adds an intro sentence to the level0 summary of a page. Currently, it will only affect Ruby docs as it depends on a "friendlyApiName" key being present in docfx yaml.
Title updates will be merged separately (see: #367), and this change will be rebased before submission.