About modal directive and unit tests

[dlabrecq]

PTNFLY-1011 About Screen - AngularJS

The pfAboutModal directive wraps UI Bootstrap’s modal, which (as is) only supports either template or templateUrl as a way to specify the content. When content is retrieved, it is compiled and linked against the provided scope by the $modal service. Unfortunately, there is no way to provide transclusion there.

The solution for pfAboutModal embeds a placeholder directive (i.e., pfAboutModalTransclude) to append the transcluded DOM. The transcluded DOM is from a different location than the modal, so it needs to be handed over to the placeholder directive.

[dlabrecq]

@jeff-phillips-18 @dtaylor113 please take a look.

[jeff-phillips-18]

Try to put $watch calls in the link function so that it is executed after the compile.

[dtaylor113]

Hmm...I thought $watch goes in controller, and $observe goes in the link function?

[jeff-phillips-18]

Better to be in link so that it occurs after compilation.

[jeff-phillips-18]

Could we add the capability to add list of product/versions to aid with their display? In the simplest case this would require not additional html by the application.

[dtaylor113]

LGTM. Appreciate the screenshot in the PR :-)

[dlabrecq]

Thanks for the comments.

@jeff-phillips-18 In regard to product/versions, I'm not quite sure what you're looking for?

[jeff-phillips-18]

I'm thinking an array of objects containing productTitle and productVersion fields. Then the directive could iterate over them and display them appropriately saving the app from having to transclude some built up html.

[dlabrecq]

So, get rid of the transclude? I used this thinking this would be a customizable area. Although, I didn't get any mockups.

I'll check with @leslie to see if this area needs to customizable. Otherwise, I'll replace the transclude with an array.

[jeff-phillips-18]

I would leave the transclude for the case where an applications wants special text/formatting.

Something like:

<div class="modal-body">
  <h1 ng-if="title">{{title}}</h1>
  <div class="product-versions-pf">
    <ul ng-if="productInfo && productInfo.length > 0" class="list-unstyled">
      <li ng-repeat="info in productInfo"><strong>{{info.product}}</strong> {{info.version}}</li>
    </ul>
    <div pf-about-modal-transclude="template"></div>
  <div ng-if="copyright" class="trademark-pf">{{copyright}}</div>
</div>

[dlabrecq]

I was just thinking we can keep the transclude. Thanks for the code!

[dlabrecq]

I updated the watch and added support for an array of objects containing product title and version info. I also added support for a two column layout (i.e., for an array length > 4) per the design spec. The transclude was left in place for custom layouts.

FYI, the two column layout depends on patternfly/patternfly/pull/428.

@jeff-phillips-18 please review/merge.

[jeff-phillips-18]

I'm not sure why the two column layout is necessary. I think it should be left as a single column as the version info may be long.

Actually, thinking about it now, I'm not sure product/version is the correct terminology (sorry my bad). We should probably call in name/value since it is likely products will list more than just products and versions (ManageIQ for instance lists server name, username, browser OS, etc).

The additional info could just be a paragraph of text explaining or expanding upon the version or copyright. So, I'm thinking more along the lines of:

[dlabrecq]

@jeff-phillips-18 Regarding the two columns, I was following the design document which described the layout with a couple statements and showed screenshots. However, after speaking with @LHinson, she agreed it should have been one column and the design doc is incorrect.

That said, I've made the name/value changes and added an additionalInfo attribute. The custom example now mimics the screen shot you provided above. All good feedback -- thanks!

[jeff-phillips-18]

LGTM