Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip: feat(linked logo section) - Build Linked Logo Section pattern #5464

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Feb 3, 2025

Done

Builds the linked logo section pattern/HOC

Fixes WD-17120

QA

  • Open demo
  • [Add QA steps]
  • Review updated documentation:
    • [List any updated documentation for review]

Check if PR is ready for release

If this PR contains Vanilla SCSS or macro code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention
    • if existing APIs (CSS classes & macro APIs) are not changed it can be a bugfix release (x.x.X)
    • if existing APIs (CSS classes & macro APIs) are changed/added/removed it should be a minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) or macros should be listed on the what's new page.

Screenshots

image
image
image

@jmuzina jmuzina added the Feature 🎁 New feature or request label Feb 3, 2025
@webteam-app
Copy link

@jmuzina
Copy link
Member Author

jmuzina commented Feb 3, 2025

Built the bones of this one ahead of the spec being fully finished in order to do some research on how the API might work before I submit the engineering side of the spec.

Our usual method of building list-style HOCs (established here) is to create slots for each type of item content from [1..n] where n is the maximum number of elements allowed for the pattern, like so:

{%- if slot == 'list_item_title_1' -%}
<h3 class="p-heading--5">Rich portfolio of products</h3>
{%- endif -%}

{%- if slot == 'list_item_description_1' -%}
<p>Resell the full range of Canonical’s portfolio of private and public
  cloud products: OpenStack, Kubernetes, IoT, support, security and
  compliance for Ubuntu.</p>
{%- endif -%}

{%- if slot == 'list_item_title_2' -%}
<h3 class="p-heading--5">Sales enablement</h3>
{%- endif -%}

{%- if slot == 'list_item_description_2' -%}
<p>Canonical can help train your field sales and presales teams and
  provide you with sales collateral and permission to use the Canonical
  and Ubuntu trademarks in your own materials.</p>
{%- endif -%}

{%- if slot == 'list_item_title_3' -%}
<h3 class="p-heading--5">Access to Canonical partner portal</h3>
{%- endif -%}

{%- if slot == 'list_item_description_3' -%}
<p>We host a partner portal with product and solutions collaterals, agreed
  pricing, incentives and a deal registration system.</p>
{%- endif -%}
<!-- and so on...-->

This does not trivially work for the linked logo section, because each item must be an <a> that contains other parts of the item as children. If the user passed in list_item_n as a wholesale slot, they wouldn't really be getting much value out of the HOC as they're constructing most of the markup themselves instead of the HOC handling it.

I.E., consider this to be the full HTML of one of the items:

<a href="#" aria-label="Kubeflow">
  <div class="p-image-container--16-9 is-highlighted">
    <img class="p-image-container__image" src="https://assets.ubuntu.com/v1/cd89477e-kubeflow-logo-container-vert-fill.png" alt="Kubeflow logo" width="432" height="481">
  </div>
  <hr class="p-rule--muted">
  <p>Learn more ›</p>
</a>

Under the old approach, the consuming developer would need to do one of the following:

  1. Create slots for the attributes of each link, i.e.:
{% if slot == "link_1" %}
  href="#" aria-label="Kubeflow"
{% endif %}
  1. Create a slot per attribute of each link:
{% if slot == "link_1_href" %}
#
{% endif %}
{% if slot == "link_1_label" %}
Kubeflow
{% endif %}

Both of these feel a bit clumsy. So, I did some exploration of an alternate JSON-based approach to provide a bit more structure to the macro and improve the developer experience.

The resulting consumer code looks like this:

{{ vf_linked_logo_section(
  title_text="PostgreSQL, security and support",
  links=[
    {
    "href": "#",
    "text": "Learn more &rsaquo;",
    "label": "Apache Spark",
    "image_html": image(url="https://assets.ubuntu.com/v1/855deda8-apache-spark-logo-container-vert-fill.png",
      alt="",
      width="240",
      height="481",
      hi_def=True,
      loading="auto",
      attrs={"class": "p-image-container__image"}
    ) | safe
  }
  ],
) }}

@jmuzina jmuzina force-pushed the ft-linked-logo-section branch from 9768c21 to d1546d1 Compare February 3, 2025 22:58
@bartaz
Copy link
Member

bartaz commented Feb 4, 2025

"image_html": image(url="https://assets.ubuntu.com/v1/855deda8-apache-spark-logo-container-vert-fill.png",
      alt="",
      width="240",
      height="481",
      hi_def=True,
      loading="auto",
      attrs={"class": "p-image-container__image"}
    ) | safe
  }

This makes me think if it would make sense to pass individual image attributes instead of image_html and pass them to image internally in the pattern. The downside would be the dependency on image template being available (and I'm not sure if we can enforce that on downstream projects), but it would have a benefit of additionally simplifying the API and removing redundant props.

It could look like this, with nested image props:

{{ vf_linked_logo_section(
  title_text="PostgreSQL, security and support",
  links=[
    {
    "href": "#",
    "text": "Learn more &rsaquo;",
    "label": "Apache Spark",
    "image": {
      url="https://assets.ubuntu.com/v1/855deda8-apache-spark-logo-container-vert-fill.png",
      alt="",
      width="240",
      height="481",
      }
  }
  ],
) }}

Or flattened:

{{ vf_linked_logo_section(
  title_text="PostgreSQL, security and support",
  links=[
    {
    "href": "#",
    "text": "Learn more &rsaquo;",
    "label": "Apache Spark",
     "image_src"="https://assets.ubuntu.com/v1/855deda8-apache-spark-logo-container-vert-fill.png",
     "image_alt"="",
     "image_width"="240",
      "image_height"="481",
      }
  }
  ],
) }}

@jmuzina
Copy link
Member Author

jmuzina commented Feb 4, 2025

This makes me think if it would make sense to pass individual image attributes instead of image_html and pass them to image internally in the pattern. The downside would be the dependency on image template being available (and I'm not sure if we can enforce that on downstream projects), but it would have a benefit of additionally simplifying the API and removing redundant props.

I thought of doing exactly this, but arrived at my current approach due to exactly what you said there: it would enforce the image template helper onto downstream projects. Passing in the image_html has the benefit of being tooling-agnostic, as any valid HTML passed in will work.

IDEs also do a fine job at recognizing this raw HTML and offering autocomplete and syntax highlighting, which I was worried about:

Screenshot 2025-02-04 at 09 15 50 Screenshot 2025-02-04 at 09 15 54

@bartaz
Copy link
Member

bartaz commented Feb 4, 2025

I thought of doing exactly this, but arrived at my current approach due to exactly what you said there: it would enforce the image template helper onto downstream projects. Passing in the image_html has the benefit of being tooling-agnostic, as any valid HTML passed in will work.

There is definitely a value in being tool-agnostic. And this is the main drawback I see and I don't know what consequences it would have to require the image from downstream projects.

There is probably less value in the field being free HTML. Because in such pre-defined pattern, do we want it to be free HTML? It seems we are expecting it to be an image, with a specific class name on top of it. This could benefit from being abstracted.

But there is obviously a drawback of relying on image template module.

There is a third option: to take image props directly and apply them in the pattern to <img> element (instead of going through image module), but that has a drawback of not using cloudinary and other built-in functionalities.

@jmuzina
Copy link
Member Author

jmuzina commented Feb 5, 2025

Talked with @immortalcodes about this yesterday. He raised similar points as @bartaz but we arrived at raw HTML for the image as being the best approach, as sites generally uses the image template this must be compatible with the image template & cloudinary in order to create the least friction in their work. We should also avoid creating downstream dependencies on the image template.

The usage of the raw HTML should be adequately documented such that its usage (and expectation of p-image-container__image class) is clear.

In the future, when we are using React-based HOCs, this will become much simpler as we will be able to take advantage of more robust input handling and controls. This approach is a good medium-term solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature 🎁 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants