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

feat(ruby): display friendly api name before page title for Ruby docs #367

Merged
merged 19 commits into from
Apr 27, 2022

Conversation

dansaadati
Copy link
Contributor

@dansaadati dansaadati commented Apr 4, 2022

This change modifies the template to prepend the "friendlyApiName" in the <h1> of generated pages. This also updates the Ruby test yaml and accompanying golden doc set.

@dansaadati dansaadati requested a review from a team April 4, 2022 22:14
@dansaadati dansaadati added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 4, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 4, 2022
Copy link
Contributor

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

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

Could you remove the unnecessary files? For the purpose of showing the friendly title, YAML updates are fine but it's containing a bit more than we should need for this PR, making it harder to focus on the necessary changes.

@@ -13,6 +13,7 @@ exports.transform = function (model) {
}

model._disableToc = model._disableToc || !model._tocPath || (model._navPath === model._tocPath);
model._displayFriendlyApiName = model._displayFriendlyApiName && model.friendlyApiName
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already have {{#_displayFriendlyApiName}} in the template, this is a redundant check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This checks that the friendlyApiName key in the yaml present and non-empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, do we need to check that it's present and non-empty for a specific reason? Suppose that this JavaScript line isn't there, if _displayFriendlyApiName is not present then the template will not render it. That's why I think this is a redundant check, unless there's a reason I'm not aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a - character between the friendly api name and the existing title. If the friendlyApiName key is missing from the yaml, the dash character will still appear unless we have this additional condition.

@dansaadati dansaadati force-pushed the title-with-friendly-api-name branch from d44ce1c to 99f2b83 Compare April 5, 2022 17:38
@dansaadati
Copy link
Contributor Author

Could you remove the unnecessary files? For the purpose of showing the friendly title, YAML updates are fine but it's containing a bit more than we should need for this PR, making it harder to focus on the necessary changes.

Yes, we can do the updates to the Ruby yaml and golden doc set as a follow-up.

@dansaadati dansaadati requested a review from dandhlee April 5, 2022 17:43
Copy link
Contributor

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

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

Could you update the goldens? Looks like the slight changes triggered whitespace changes, curious how it look snow.

@@ -1,7 +1,10 @@
{{!Copyright (c) Microsoft. All rights reserved. Licensed under the MIT license. See LICENSE file in the project root for full license information.}}
{{!master(layout/_master.tmpl)}}

<h1 class="page-title">{{#title}}{{title}}{{/title}}{{^title}}{{>partials/title}}{{/title}}</h1>
<h1 class="page-title">
{{#_displayFriendlyApiName}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need _displayFriendlyApiName. Values are truthy in the template. So, we should just be able to do

{{#friendlyApiName}}
{{friendlyApiName}}
{{/friendlyApiName}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to have a flag, _displayFriendlyApiName, to either enable or disable this change from doc-pipeline. Ruby libraries are already generating yaml with the friendlyApiName key so updating the template would effectively make this change live on prod for at least some Ruby libraries. However, I think just relying on the friendlyApiName key (and not changing the docfx JSON) is okay too as this is a pretty safe change. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

To disable this experiment, we can remove this from the template, or "turn off" them on docfx.json. At the moment because there is no experiment structure they're equivalent (as in a PR for deleting/changing in this repo). In that case I'd prefer to have the template only contain what's necessary rather than leaving this behind and possibly forgetting this later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, we can just use the friendlyApiName key directly then.

@dansaadati dansaadati requested a review from tbpg April 6, 2022 22:28
@dansaadati dansaadati force-pushed the title-with-friendly-api-name branch from c0eb7a3 to 569f0cc Compare April 7, 2022 16:11
@dansaadati dansaadati force-pushed the title-with-friendly-api-name branch from 569f0cc to d7470a6 Compare April 7, 2022 16:15
@dansaadati dansaadati requested a review from dandhlee April 7, 2022 16:18
Copy link
Contributor

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -8,7 +8,9 @@
{% verbatim %}
<div>
<article data-uid="cloud.google.com/go/storage">
<h1 class="page-title">Package cloud.google.com/go/storage
<h1 class="page-title">
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way we can avoid these whitespace changes? This would modify every file. Would be nice not to. Fine if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if we keep it all in one line in the template, but it's slightly less readable:

<h1 class="page-title">{{#friendlyApiName}}{{friendlyApiName}} - {{/friendlyApiName}}{{#title}}{{title}}{{/title}}{{^title}}{{>partials/title}}{{/title}}</h1>

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is pretty intense to read. So, let's leave it as-is.

renovate-bot and others added 4 commits April 13, 2022 15:50
* 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#&#8203;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 ([#&#8203;748](https://togithub.com/googleapis/python-storage/issues/748)) ([752e8ab](https://togithub.com/googleapis/python-storage/commit/752e8ab42d23afd68738e4d7ca6cdeee416dfd50))
-   track invocation id for retry metrics ([#&#8203;741](https://togithub.com/googleapis/python-storage/issues/741)) ([bd56931](https://togithub.com/googleapis/python-storage/commit/bd5693164e7331df5f14186fd002e72e5203d7ee))

##### Bug Fixes

-   **deps:** drop pkg_resources ([#&#8203;744](https://togithub.com/googleapis/python-storage/issues/744)) ([e963f33](https://togithub.com/googleapis/python-storage/commit/e963f33ced2852b64d721d69928b54443461ec9c))

##### Documentation

-   fix links in blob module ([#&#8203;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 ([#&#8203;735](https://togithub.com/googleapis/python-storage/issues/735)) ([f77d2f7](https://togithub.com/googleapis/python-storage/commit/f77d2f787f435f2f898e9babcdab81225672ad4f)), closes [#&#8203;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).
@dansaadati dansaadati merged commit 7fb7d91 into main Apr 27, 2022
@dansaadati dansaadati deleted the title-with-friendly-api-name branch April 27, 2022 22:32
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.

5 participants