changed the add documents and reorder links from p tags to list tags …

[RodneyJohnsonGDS]

…as these are more semantically and stylistally correct

This commit is a continuation from a previously closed and merged branch, can be founder here:
#8318 (comment)

This commit user story can be found in this Trello link: https://trello.com/c/kEcGRQ1X/531-allow-for-the-reordering-of-documents-within-a-group

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

[davidtrussler]

@RodneyJohnsonGDS I think that markup is more appropriate now and consistent but I think the use of display: flex here is a little problematic as it is aligning the links centrally to the header and distorting their vertical alignment, albeit only marginally, so I would prefer to style it without that.

Also, we should probably be a bit more generic in the way the margins and borders for the links are applied so that it could potentially work for single or multiple links.

I'll suggest an alternative in the CSS file.

[davidtrussler]

I think it would be better to lose the whole app-view-document-collection-group-memberships-index__heading-links class and add a more generic style nested in the app-view-document-collection-group-memberships-index__heading element like below.

That way we can just allow the <li> elements to flow more naturally and add the separating border in a way that doesn't rely on a specific number of links to be present (as it is it would add a rule to the single link for "Add document" when there are none to display). It also centres the vertical rule better and keeps the horizontal alignment with the header.

I think I'd also lose the flexbox layout in the containing element as well because that makes it display oddly on smaller viewports. By floating the <ul> element we can let it flow underneath the heading in this case, though we do need to change the heading from a block level element to allow this, and give it a bit of a margin to avoid the elements clashing.

Once we're nested inside the very specific app-view-document-collection-group-memberships-index__heading element I think we can afford to be far more generic in the way we reference elements so I don't think we need to add specific classes for those.

See what you think. I know it's quite a big change but it makes it closer to the sort of styles we are using in related components. (Also I think flexbox can be overused. 😄)

.app-view-document-collection-group-memberships-index__heading {
  .gem-c-heading {
    display: inline-block;
    @include govuk-responsive-margin(2, "right");
  }

  .govuk-list {
    float: right; 

    li {
      display: inline-block;
      @include govuk-responsive-padding(2, "left");
      @include govuk-responsive-margin(2, "left");
      border-left: 1px solid $govuk-border-colour;

      &:first-child {
        margin-left: 0;
        padding-left: 0;
        border-left: none;
      }
    }
  }
}

[davidtrussler]

If we use the approach above we can lose the app-view-document-collection-group-memberships-index__heading-links class here.

[davidtrussler]

Again, if we use the above approach we can lose the govuk-!-text-align-right class here, and we shouldn't have the govuk-body class at all.

[RodneyJohnsonGDS]

Looks good, done!

[davidtrussler]

This would need to be more generic to catch all instances of this file as it occurs in many directories and subdirectories (e.g. the one in the app directory has survived). I'm not sure we should need to add this though if we make sure we don't commit those files.

[davidtrussler]

LGTM 👍