launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/registry/browser/tests/test_pillar_sharing.py b/lib/lp/registry/browser/tests/test_pillar_sharing.py
> index 493b42b..215f946 100644
> --- a/lib/lp/registry/browser/tests/test_pillar_sharing.py
> +++ b/lib/lp/registry/browser/tests/test_pillar_sharing.py
> @@ -172,8 +172,9 @@ class PillarSharingDetailsMixin:
>              pillarperson.pillar.name, pillarperson.person.name)
>          browser = self.getUserBrowser(user=self.owner, url=url)
>          self.assertIn(
> -            'There are no shared bugs, Bazaar branches, Git repositories, or '
> -            'blueprints.', normalize_whitespace(browser.contents))
> +            'There are no shared bugs, Bazaar branches, Git repositories, '
> +            'Snaps, OCI recipes or blueprints.',

We normally set "snaps" in sentence case (so lower-case here), and consider "snap recipes" instead.

> +            normalize_whitespace(browser.contents))
>  
>      def test_init_works(self):
>          # The view works with a feature flag.
> diff --git a/lib/lp/registry/javascript/sharing/sharingdetails.js b/lib/lp/registry/javascript/sharing/sharingdetails.js
> index 0ac4f32..60b56d5 100644
> --- a/lib/lp/registry/javascript/sharing/sharingdetails.js
> +++ b/lib/lp/registry/javascript/sharing/sharingdetails.js
> @@ -148,6 +148,58 @@ ns.SharingDetailsTable = Y.Base.create('sharingDetailsTable', Y.Widget, [], {
>          ].join(' ');
>      },
>  
> +    _snap_details_row_template: function() {
> +        return [
> +        '<tr id="shared-snap-{{id}}">',
> +        '    <td>',
> +        '        <span class="sortkey">{{id}}</span>',
> +        '        <a class="sprite source-package-recipe" href="{{web_link}}">',

I don't think we normally use a sprite for snaps today, do we?  source-package-recipe doesn't seem quite right for it.

> +        '            {{name}}',
> +        '        </a>',
> +        '    </td>',
> +        '    <td class="action-icons nowrap">',
> +        '    <span id="remove-snap-{{id}}">',
> +        '    <a class="sprite remove action-icon" href="#"',
> +        '        title="Unshare Snap {{name}} with {{displayname}}"',
> +        '        data-self_link="{{self_link}}" data-name="{{name}}"',
> +        '        data-type="snap">Remove</a>',
> +        '    </span>',
> +        '    </td>',
> +        '   <td>',
> +        '   <span class="information_type">',
> +        '  {{information_type}}',
> +        '   </span>',
> +        '   </td>',
> +        '</tr>'
> +        ].join(' ');
> +    },
> +
> +    _ocirecipe_details_row_template: function() {
> +        return [
> +        '<tr id="shared-ocirecipe-{{id}}">',
> +        '    <td>',
> +        '        <span class="sortkey">{{id}}</span>',
> +        '        <a class="sprite source-package-recipe" href="{{web_link}}">',

Same comment as above.

> +        '            {{name}}',
> +        '        </a>',
> +        '    </td>',
> +        '    <td class="action-icons nowrap">',
> +        '    <span id="remove-ocirecipe-{{id}}">',
> +        '    <a class="sprite remove action-icon" href="#"',
> +        '        title="Unshare OCI recipe {{name}} with {{displayname}}"',
> +        '        data-self_link="{{self_link}}" data-name="{{name}}"',
> +        '        data-type="ocirecipe">Remove</a>',
> +        '    </span>',
> +        '    </td>',
> +        '   <td>',
> +        '   <span class="information_type">',
> +        '  {{information_type}}',
> +        '   </span>',
> +        '   </td>',
> +        '</tr>'
> +        ].join(' ');
> +    },
> +
>      _table_body_template: function() {
>          return [
>          '<tbody id="sharing-table-body">',
> diff --git a/lib/lp/registry/templates/pillar-sharing-details.pt b/lib/lp/registry/templates/pillar-sharing-details.pt
> index a6f2710..0b89496 100644
> --- a/lib/lp/registry/templates/pillar-sharing-details.pt
> +++ b/lib/lp/registry/templates/pillar-sharing-details.pt
> @@ -26,20 +26,31 @@
>    <div metal:fill-slot="main">
>  
>      <div id="observer-summary">
> -      <p>
> -      <tal:bugs replace="view/shared_bugs_count">0</tal:bugs> bugs,
> -      <tal:branches replace="view/shared_branches_count">0</tal:branches> Bazaar branches,
> -      <tal:gitrepositories replace="view/shared_gitrepositories_count">0</tal:gitrepositories> Git repositories,
> -      and  <tal:specifications
> -      replace="view/shared_specifications_count">0</tal:specifications>
> -      blueprints shared with <tal:name replace="view/person/displayname">
> -      grantee</tal:name>.<br />
> -
>        <tal:is-team condition="view/person/is_team">
>          <tal:members>3</tal:members> team members can view these bugs,
> -        Bazaar branches, Git repositories, and blueprints.
> +        Bazaar branches, Git repositories, Snaps, OCI recipes and blueprints.

"snap recipes"

>        </tal:is-team>
> -      </p>
> +      Shared with <tal:name replace="view/person/displayname">grantee</tal:name>
> +      <ul class="listing">
> +        <li tal:condition="view/shared_bugs_count">
> +          <span tal:replace="view/shared_bugs_count" /> Bugs

"bugs" should be in sentence case.

> +        </li>
> +        <li tal:condition="view/shared_branches_count">
> +          <span tal:replace="view/shared_branches_count" /> Bazaar branches
> +        </li>
> +        <li tal:condition="view/shared_gitrepositories_count">
> +          <span tal:replace="view/shared_gitrepositories_count" /> Git repositories
> +        </li>
> +        <li tal:condition="view/shared_ocirecipe_count">
> +          <span tal:replace="view/shared_ocirecipe_count" /> OCI recipes
> +        </li>
> +        <li tal:condition="view/shared_snaps_count">
> +          <span tal:replace="view/shared_snaps_count" /> Snap recipes

"snap recipes" should be in sentence case.

> +        </li>
> +        <li tal:condition="view/shared_specifications_count">
> +          <span tal:replace="view/shared_specifications_count" /> Blueprints

"blueprints" should be in sentence case.

> +        </li>
> +      </ul>

Could I see before vs. after screenshots of this UI change?  I think I see what you're getting at, but screenshots would make it easier.

>      </div>
>  
>      <table id="shared-table" class="listing sortable">


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/400059
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:ocirecipe-edit-info-type-ui.