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.

Sign up for GitHub

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

Refactor summary component to fix lighthouse test #3408

Merged
merged 11 commits into from
Nov 4, 2024
Merged

Refactor summary component to fix lighthouse test #3408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c4ae1a7
refactor summary component to remove extra div in dl
rmccar Oct 28, 2024
5c4d79d
remove has-values class no longer used
rmccar Oct 28, 2024
65f5060
fix tests
rmccar Oct 28, 2024
ccf1589
fix tests
rmccar Oct 28, 2024
0b3bc20
fix tests
rmccar Oct 28, 2024
86f7a6e
mark summary titles as dts
rmccar Oct 28, 2024
7a99b42
tidy up css
rmccar Oct 28, 2024
5e2d10c
add class to container element
rmccar Oct 28, 2024
5c06975
Merge branch 'main' into refactor-summary-component-to-remove-extra-d…
rmccar Oct 29, 2024
a4951b6
Merge branch 'main' into refactor-summary-component-to-remove-extra-d…
rmccar Oct 31, 2024
7dee403
remove id from params list
rmccar Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/components/summary/_macro-options.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

| Name | Type | Required | Description |
| ---------------------- | ---------------------- | -------- | ------------------------------------------------------------------------------------------- |
| id | string | false | The HTML `id` of the row item |
| iconType | string | false | Adds an icon before the row title, by setting the [icon type](/foundations/icons#icon-type) |
| iconVisuallyHiddenText | string | false | Visually hidden text in a span under the icon to add more context for screen readers |
| title | string | false | The title for the row item |
Expand Down
153 changes: 74 additions & 79 deletions src/components/summary/_macro.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,96 +35,91 @@
{% if row.total %}{% set itemClass = itemClass + " ons-summary__item--total" %}{% endif %}
<div {% if row.id %}id="{{ row.id }}"{% endif %}class="ons-summary__item{{ itemClass }}">
{% if row.errorMessage %}
<div class="ons-summary__row-title--error ons-u-fs-r">{{ row.errorMessage }}</div>
<dt class="ons-summary__row-title--error ons-u-fs-r">{{ row.errorMessage }}</dt>
{% endif %}
{% if row.itemsList | length > 1 and row.title %}
<div class="ons-summary__row-title ons-summary__row-title--no-group-title ons-u-fs-r">
<dt class="ons-summary__row-title ons-summary__row-title--no-group-title ons-u-fs-r">
{{- row.title -}}
</div>
</dt>
{% endif %}

{% for item in row.itemsList %}
<div
class="ons-summary__row{{ ' ons-summary__row--has-values' if item.valueList else "" }}"
{% if item.id %}id="{{ item.id }}"{% endif %}
precious-onyenaucheya-ons marked this conversation as resolved.
Show resolved Hide resolved
<dt
class="ons-summary__item-title"
{% if item.titleAttributes %}{% for attribute, value in (item.titleAttributes.items() if item.titleAttributes is mapping and item.titleAttributes.items else item.titleAttributes) %}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
<dt
class="ons-summary__item-title"
{% if item.titleAttributes %}{% for attribute, value in (item.titleAttributes.items() if item.titleAttributes is mapping and item.titleAttributes.items else item.titleAttributes) %}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
{% if item.iconType %}
{% from "components/icon/_macro.njk" import onsIcon %}
<span
class="ons-summary__item-title-icon{{ ' ons-summary__item-title-icon--check' if item.iconType == 'check' else "" }}"
>
{{-
onsIcon({
"iconType": item.iconType
})
-}}
{% if item.iconVisuallyHiddenText %}
<span class="ons-u-vh">{{ item.iconVisuallyHiddenText }}</span>
{% endif %}
</span>
{% endif %}

<div
class="ons-summary__item--text{{ ' ons-summary__item-title--text' if item.iconType else "" }}"
{% if item.iconType %}
{% from "components/icon/_macro.njk" import onsIcon %}
<span
class="ons-summary__item-title-icon{{ ' ons-summary__item-title-icon--check' if item.iconType == 'check' else "" }}"
>
{{- item.title | default(row.title) | safe -}}
</div>
{{-
onsIcon({
"iconType": item.iconType
})
-}}
{% if item.iconVisuallyHiddenText %}
<span class="ons-u-vh">{{ item.iconVisuallyHiddenText }}</span>
{% endif %}
</span>
{% endif %}

{# Render section status for mobile if is hub #}
{% if variantHub and item.valueList %}
<span class="ons-u-d-no@m ons-u-fs-r"> — {{ item.valueList[0].text | safe }}</span>
{% endif %}
</dt>
{% if item.valueList %}
<dd
class="ons-summary__values{{ ' ons-summary__values--2' if not item.actions }}"
{% if item.attributes %}{% for attribute, value in (item.attributes.items() if item.attributes is mapping and item.attributes.items else item.attributes) %}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
{% if item.valueList | length == 1 %}
<span class="ons-summary__text">{{ item.valueList[0].text | safe }}</span>
{% if item.valueList[0].other or item.valueList[0].other == 0 %}
<ul class="ons-u-mb-no">
<li>{{ item.valueList[0].other | safe }}</li>
</ul>
{% endif %}
{% else %}
<div
class="ons-summary__item--text{{ ' ons-summary__item-title--text' if item.iconType else "" }}"
>
{{- item.title | default(row.title) | safe -}}
</div>

{# Render section status for mobile if is hub #}
{% if variantHub and item.valueList %}
<span class="ons-u-d-no@m ons-u-fs-r"> — {{ item.valueList[0].text | safe }}</span>
{% endif %}
</dt>
{% if item.valueList %}
<dd
class="ons-summary__values{{ ' ons-summary__values--2' if not item.actions }}"
{% if item.attributes %}{% for attribute, value in (item.attributes.items() if item.attributes is mapping and item.attributes.items else item.attributes) %}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
{% if item.valueList | length == 1 %}
<span class="ons-summary__text">{{ item.valueList[0].text | safe }}</span>
{% if item.valueList[0].other or item.valueList[0].other == 0 %}
<ul class="ons-u-mb-no">
{% for value in item.valueList %}
<li>
<span class="ons-summary__text">{{ value.text | safe }}</span>
{% if value.other or value.other == 0 %}
<ul class="ons-u-mb-no">
<li>{{ value.other | safe }}</li>
</ul>
{% endif %}
</li>
{% endfor %}
<li>{{ item.valueList[0].other | safe }}</li>
</ul>
{% endif %}
</dd>
{% endif %}
{% if item.actions %}
<dd class="ons-summary__actions">
{% for action in item.actions %}
{% if loop.index > 1 %}<span class="ons-summary__spacer"></span>{% endif %}
<a
href="{{ action.url }}"
class="ons-summary__button"
{% if action.attributes %}{% for attribute, value in (action.attributes.items() if action.attributes is mapping and action.attributes.items else action.attributes) %}{{ ' ' }}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
<span class="ons-summary__button-text" aria-hidden="true">
{{- action.text -}}
</span>
<span class="ons-u-vh">{{ action.visuallyHiddenText }}</span>
</a>
{% endfor %}
</dd>
{% endif %}
</div>
{% else %}
<ul class="ons-u-mb-no">
{% for value in item.valueList %}
<li>
<span class="ons-summary__text">{{ value.text | safe }}</span>
{% if value.other or value.other == 0 %}
<ul class="ons-u-mb-no">
<li>{{ value.other | safe }}</li>
</ul>
{% endif %}
</li>
{% endfor %}
</ul>
{% endif %}
</dd>
{% endif %}
{% if item.actions %}
<dd class="ons-summary__actions">
{% for action in item.actions %}
{% if loop.index > 1 %}<span class="ons-summary__spacer"></span>{% endif %}
<a
href="{{ action.url }}"
class="ons-summary__button"
{% if action.attributes %}{% for attribute, value in (action.attributes.items() if action.attributes is mapping and action.attributes.items else action.attributes) %}{{ ' ' }}{{ attribute }}="{{ value }}"{% endfor %}{% endif %}
>
<span class="ons-summary__button-text" aria-hidden="true">
{{- action.text -}}
</span>
<span class="ons-u-vh">{{ action.visuallyHiddenText }}</span>
</a>
{% endfor %}
</dd>
{% endif %}
{% endfor %}
</div>
{% endfor %}
Expand All @@ -146,7 +141,7 @@
</a>
{% endset %}
{% if params.variant == 'card' %}
<div>{{ link | safe }}</div>
<div class="ons-summary__link-container">{{ link | safe }}</div>
{% else %}
{{ link | safe }}
{% endif %}
Expand Down
20 changes: 5 additions & 15 deletions src/components/summary/_macro.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,12 +399,6 @@ describe('macro: summary', () => {
expect($('#row-id-3').length).toBe(1);
});

it('has the correct class for each row when there is a `valueList`', () => {
const $ = cheerio.load(renderComponent('summary', EXAMPLE_SUMMARY_BASIC));

expect($('.ons-summary__row--has-values').length).toBe(5);
});

it('has custom row `titleAttributes`', () => {
const $ = cheerio.load(renderComponent('summary', EXAMPLE_SUMMARY_BASIC));

Expand Down Expand Up @@ -462,18 +456,16 @@ describe('macro: summary', () => {
const $ = cheerio.load(renderComponent('summary', EXAMPLE_SUMMARY_BASIC));

expect(
$('.ons-summary__items .ons-summary__item:nth-of-type(1) .ons-summary__row .ons-summary__values .ons-summary__text')
.text()
.trim(),
$('.ons-summary__items .ons-summary__item:nth-of-type(1) .ons-summary__values .ons-summary__text').text().trim(),
).toBe('row value 1');
});

it('displays the `other` text', () => {
const $ = cheerio.load(renderComponent('summary', EXAMPLE_SUMMARY_BASIC));

expect(
$('.ons-summary__items .ons-summary__item:nth-of-type(1) .ons-summary__row .ons-summary__values ul li').text().trim(),
).toBe('other value');
expect($('.ons-summary__items .ons-summary__item:nth-of-type(1) .ons-summary__values ul li').text().trim()).toBe(
'other value',
);
});

it('wraps the `valueList` in a ul if multiple values provided', () => {
Expand Down Expand Up @@ -601,9 +593,7 @@ describe('macro: summary', () => {
}),
);

expect($('.ons-summary__items .ons-summary__item:nth-of-type(2) .ons-summary__row .ons-summary__item-title span').text()).toBe(
' — row value 2',
);
expect($('.ons-summary__items .ons-summary__item:nth-of-type(2) .ons-summary__item-title span').text()).toBe(' — row value 2');
});
});

Expand Down
Loading