Four Kitchens Release - Sprint 40 #1415

cienvaras · 2023-12-05T15:33:41Z

READY FOR REVIEW

Summary

SHS-5149: A11y Empty/nested headings on views cards
SHS-5150: A11y Empty/nested headings in Views groupings
SHS-5151: A11y empty headings on view templates for LEGACY theme
SHS-5134: Photo Album links have unhelpful aria-label
SHS-4981: [EX] Dynamically change label for hs_hero_image paragraph type based on context
SHS-5154: Layout Builder Styling

Need Review By (Date)

12/15

Urgency

medium

Steps to Test

SHS-5149

Visit the following links:
https://chemistry.suhumsci.loc/people/lecturers
https://jewishstudies.suhumsci.loc/academics/awards-and-opportunities
https://mrc.suhumsci.loc/videos
https://francestanford.suhumsci.loc/project-area-study/humanities-arts
Ensure that they are no longer producing empty heading tags (compare to:
https://chemistry.stanford.edu/people/lecturers
https://jewishstudies.stanford.edu/academics/awards-and-opportunities
https://mrc.stanford.edu/videos
https://francestanford.stanford.edu/project-area-study/humanities-arts)

SHS-5150

Ensure there are no longer WAVE issues around empty heading tags printing on the following pages:

https://anthropology.suhumsci.loc/people/faculty-and-affiliates/faculty/leadership
https://dlcl.suhumsci.loc/people/faculty
https://shenlab.stanford.edu/people

Also check:
https://hs-traditional.suhumsci.loc/default-views/people

Please note that I have found one outlying issue that is noticeable in the traditional example. If the view returns "grouped" content, and it is grouped by a field that allows the user to select nothing, the field will print with comments inside the <h3>. I believe this will only happen live when logged in (I'm having trouble testing that since I can see template comments logged out locally.)

SHS-5151

The issue can be seen in this site:
https://mathematics.stanford.edu/people/department-administration
Visit the same page locally https://mathematics.suhumsci.loc/people/department-administration to see the changes fix in place. If you are logged in, you will no longer see the issue in Editorially.

SHS-5134

Visit an example page with a Photo Gallery like http://hs-colorful.suhumsci.loc/components/photo-album
The thumbnail gallery images should be links. In the markup, the links should have an aria-title attribute. This attribute should be (in this order): "Open [ALT TEXT] image in the gallery" if the alt text isn't blank, or generic text "Open image in the gallery" if neither of those exist.

SHS-4981

Login as an administrator
Create a new flexible page
Add an hs_carousel (Banner image(s) with text box) component
Verify the component's Add button has the text Add Banner image with text box
Verify the Paragraph type label for all items within the hs_carousel component have the text Banner image with text box
Add an hs_hero_image (Banner image with full overlay and text) component
Verify the component's Paragraph type label has the text Banner image with full overlay and text

SHS-5154

Visit a page that uses layout builder (like the homepage)
Edit the homepage and click layout
Verify that checkboxes have been themed to be smaller and next to their label
Verify that the revision information detail element is closed by default
Note that the fields and labels are better distinguished (grouped together, with more margin between each pair.)

PR Checklist

* fix(SHS-5134): fixing titles on colorbox galleries * fix(shs-5134): adjustments to aria-labels on galleries * fix(SHS-5134): removing custom data-cbox-img-attrs value * fix(shs-5134): simplifying alt attribute

* fix(shs-5150): fixing empty headings on grid views * fix(shs-5149): ensuring that empty card title tags are not printing * fix(shs-5149): removing unused code * fix(shs-5149): removing unused code * feat(shs-5149): add content heading check to date-stacked-vertical-card pattern --------- Co-authored-by: Andrés Díaz Soto <[email protected]>

…ype based on context (#1417) * feat(shs-4981): update labels for hs_carousel form and embedded paragraph types * feat(shs-4981): add use statement * feat(shs-4981): fix codeclimate issues * feat(shs-4981): check if array offsets exist before accessing them * fix(shs-4981): typo --------- Co-authored-by: Andrés Díaz Soto <[email protected]>

* fix(shs-5154): theming adjustments to layout builder forms * fix(shs-5154): close revision information details by default * fix(shs-5154): fixing code climate errors * fix(shs-5154): refining selector for revision info js --------- Co-authored-by: Andrés Díaz Soto <[email protected]>

pookmish · 2024-01-08T16:22:41Z

docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.module

+/**
+ * Implements hook_field_widget_single_element_WIDGET_TYPE_form_alter().
+ */
+function hs_paragraph_types_field_widget_single_element_paragraphs_form_alter(&$element, &$form_state, $context) {


$element and $context should be typehinted.
$form_state is a FormStateInterface. It should be typehinted and doesn't need to be passed by reference.

Moved to hs_paragraph_types_field_widget_complete_paragraphs_form_alter and fixed there

pookmish · 2024-01-08T16:22:54Z

docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.module

+ */
+function hs_paragraph_types_field_widget_single_element_paragraphs_form_alter(&$element, &$form_state, $context) {
+  // Change the paragraph type label for hs_hero_image embedded in hs_carousel.
+  if ($element['#paragraph_type'] === 'hs_hero_image' && $element['#bundle'] === 'hs_carousel') {


No need for the triple = here. Double is enough and is consistent with out normal code practice

pookmish · 2024-01-08T16:24:32Z

docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.module

+/**
+ * Implements hook_field_widget_single_element_WIDGET_TYPE_form_alter().
+ */
+function hs_paragraph_types_field_widget_single_element_paragraphs_form_alter(&$element, &$form_state, $context) {


Change this to a hook_field_widget_complete_form_alter. See the comment in the API documentation: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/field/field.api.php?ref_type=heads#L231

pookmish · 2024-01-08T16:25:10Z

docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.module

+  // Change the add button label for field_hs_carousel_slides.
+  if (
+    isset($field_widget_form['widget']['add_more']['add_more_button_hs_hero_image'], $field_widget_form['widget']['#field_name']) &&
+    $field_widget_form['widget']['#field_name'] === 'field_hs_carousel_slides'


No need for the triple = here. Double is enough and is consistent with out normal code practice

pookmish · 2024-01-08T16:27:10Z

docroot/themes/humsci/su_humsci_theme/templates/views/views-view-grid.html.twig

+<div{{ attributes.addClass(classes) }}>
+  {% if options.alignment == 'horizontal' %}
+    {% for row in items %}
+      <div{{ row.attributes.addClass(row_classes, options.row_class_default ? 'row-' ~ loop.index) }}>
+        {% for column in row.content %}
+          <div{{ column.attributes.addClass(col_classes, options.col_class_default ? 'col-' ~ loop.index) }}>


What are the thoughts on changing this to <ul> and <li> elements? View results are typically lists and we should present that to screen readers as such.

@pookmish Yes, this is definitely a good improvement and something we can check and implement in traditional and colorful themes. We'd prefer to keep this particular template as is, because the legacy theme is going to be phased out soon. We just added these templates to fix a specific a11y issue in mathematics, but the site will soon be migrated to one of the new themes.

…stnfd-sprint-40

cienvaras · 2024-01-12T22:33:15Z

@pookmish All issues fixed, ready for review. Thanks for the feedback!

pookmish

There's only one thing I want to verify.

pookmish · 2024-01-19T20:35:10Z

docroot/modules/humsci/hs_paragraph_types/hs_paragraph_types.module

+    foreach ($field_widget_form['widget'] as $key => $element) {
+      // Only alter markup in field widget elements.
+      if (!is_int($key)) {
+        continue;
+      }
+      $element['top']['type']['label']['#markup'] = $new_label;
+      $field_widget_form['widget'][$key] = $element;


Don't you need to loop by reference if you want to change the label? &$element

Oh now that i read it more closely, it would be alot better to loop by reference and remove line 57 entirely.
Also, a better way (and more consistent with core code) to loop on the keys is to use \Drupal\Core\Render\Element::children($field_widget_form['widget']). This will give you the keys and you can check for values in the arrays.

@pookmish Fixed. PR is ready for review again.

…stnfd-sprint-40

… hs_carousel components

Shs 5134 photo album aria (#1404)

915dbe3

* fix(SHS-5134): fixing titles on colorbox galleries * fix(shs-5134): adjustments to aria-labels on galleries * fix(SHS-5134): removing custom data-cbox-img-attrs value * fix(shs-5134): simplifying alt attribute

cienvaras changed the base branch from develop to 10.0.5-release December 5, 2023 15:33

mndonx and others added 5 commits December 5, 2023 09:53

fix(shs-5150): fixing empty headings on grid views (#1413)

57d2121

fix(shs-5151): fixing duplicated heading tags in legacy theme (#1416)

e2296b8

cienvaras marked this pull request as ready for review December 14, 2023 03:42

Base automatically changed from 10.0.5-release to develop December 14, 2023 18:47

cienvaras changed the base branch from develop to 10.0.6-release December 14, 2023 18:53

cienvaras requested review from joegl and pookmish December 14, 2023 18:54

cienvaras assigned joegl and pookmish Dec 14, 2023

cienvaras added the Needs review label Dec 14, 2023

Base automatically changed from 10.0.6-release to develop December 14, 2023 18:58

cienvaras changed the base branch from develop to 10.0.7-release December 14, 2023 19:12

pookmish requested changes Jan 8, 2024

View reviewed changes

hectorlj and others added 4 commits January 10, 2024 09:29

fix: move to correct form alter hook

9cfc810

fix: add missing typehints

4603c36

fix: update logic code style

59fa48a

Merge branch '10.0.7-release' of github.com:SU-HSDO/suhumsci into fk-…

f84d892

…stnfd-sprint-40

cienvaras requested a review from pookmish January 12, 2024 22:32

pookmish reviewed Jan 19, 2024

View reviewed changes

cienvaras added 2 commits January 19, 2024 17:54

Merge branch '10.0.7-release' of github.com:SU-HSDO/suhumsci into fk-…

2e1524e

…stnfd-sprint-40

fix(shs-4981): improve handling of child component rename when adding…

aea129a

… hs_carousel components

cienvaras requested a review from pookmish January 22, 2024 15:11

pookmish approved these changes Jan 22, 2024

View reviewed changes

pookmish merged commit 14b85e6 into 10.0.7-release Jan 22, 2024
7 of 8 checks passed

pookmish deleted the fk-stnfd-sprint-40 branch January 22, 2024 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Four Kitchens Release - Sprint 40 #1415

Four Kitchens Release - Sprint 40 #1415

cienvaras commented Dec 5, 2023 •

edited

Loading

pookmish Jan 8, 2024

hectorlj Jan 10, 2024

pookmish Jan 8, 2024

hectorlj Jan 10, 2024

pookmish Jan 8, 2024

hectorlj Jan 10, 2024

pookmish Jan 8, 2024

hectorlj Jan 10, 2024

pookmish Jan 8, 2024

cienvaras Jan 12, 2024

cienvaras commented Jan 12, 2024

pookmish left a comment

pookmish Jan 19, 2024

pookmish Jan 19, 2024

cienvaras Jan 22, 2024

Four Kitchens Release - Sprint 40 #1415

Four Kitchens Release - Sprint 40 #1415

Conversation

cienvaras commented Dec 5, 2023 • edited Loading

READY FOR REVIEW

Summary

Need Review By (Date)

Urgency

Steps to Test

SHS-5149

SHS-5150

SHS-5151

SHS-5134

SHS-4981

SHS-5154

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cienvaras commented Jan 12, 2024

pookmish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cienvaras commented Dec 5, 2023 •

edited

Loading