8000 MWPW-170454 : Fixed visual list is not marked up as list and footer issue by hkuraware · Pull Request #4177 · adobecom/milo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MWPW-170454 : Fixed visual list is not marked up as list and footer issue #4177

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

Merged
merged 7 commits into from
Jun 2, 2025
Merged

MWPW-170454 : Fixed visual list is not marked up as list and footer issue #4177

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
629967c
fixed visual list is not marked up as list a11y issue
hkuraware May 6, 2025
75291ea
fixed eslint
hkuraware May 7, 2025
e0c31f0
Fixed footer alignment
hkuraware May 16, 2025
d7176e8
Merge branch 'adobecom:stage' into mwpw-170454
hkuraware May 19, 2025
5c8a78b
Merge branch 'stage' into mwpw-170454
hkuraware May 27, 2025
43869e0
refactored the code
hkuraware May 29, 2025
ed5a794
fixed review comments
hkuraware May 29, 2025
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
20 changes: 17 additions & 3 deletions libs/blocks/global-navigation/utilities/menu/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ const decorateElements = ({ elem, className = 'feds-navLink', itemIndex = { posi

// If the element is a link, decorate it and return it directly
if (elem.matches(linkSelector)) {
return decorateLink(elem);
return toFragment`<li>${decorateLink(elem)}</li>`;
}

// Otherwise, this might be a collection of elements;
Expand Down Expand Up @@ -282,14 +282,28 @@ const decorateColumns = async ({ content, separatorTagName = 'H5' } = {}) => {

itemDestination.append(imageElem);
} else {
const decoratedElem = decorateElements({ elem: columnElem, itemIndex });
let decoratedElem = decorateElements({ elem: columnElem, itemIndex });
columnElem.remove();

// If an items template has been previously created,
// add the current element to it;
// otherwise append the element to the section
const elemDestination = menuItems || itemDestination;
elemDestination.append(decoratedElem);
let menuList = null;
if (decoratedElem.tagName === 'P') {
decoratedElem = toFragment`<li>${decoratedElem.innerHTML}</li>`;
}
if (decoratedElem.tagName === 'LI') {
let ul = elemDestination.querySelector('ul');
if (!ul) {
ul = toFragment`<ul></ul>`;
elemDestination.append(ul);
}
menuList = ul;
} else {
menuList = elemDestination;
}
menuList.append(decoratedElem);
Comment on lines +292 to +306
Copy link
Contributor
@sharmrj sharmrj May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Couple of quick questions

Should this logic live inside the decorateElements function? I see that decorateElements already has logic related to the parent tag inside the decorateLink closure.
Similarly, should decorateLinkGroup always return <li>${content}</li> directly instead of doing the wrapping at the callsite?
Or is it that those changes are too general for what we're trying to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharmrj Fixed it now. Please review.
@spadmasa Please re-test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see you made a change to one of the things I pointed out, but I wasn't asking for a change. I wanted to know why we'd do <li>${someFunction()}</li> instead of returning the lis from the function itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharmrj Yes, but this fix is necessary for all links returned by decorateElements. The decorateLinkGroup function, which is exported from menu.js, is specifically used to decorate link groups. Depending on the context, it may be used with or without an <li> element. To handle this, I'm wrapping the output of decorateLinkGroup in an <li> wherever it's needed. However, it can still function as a standalone decorated link when the <li> wrapper isn't required—like we’ve done previously in the footer. Overall, the goal is for decorateLinkGroup to handle only the decoration of the link group, while the decision to wrap it in <li> or not will be made wherever it is used.

}
}

Expand Down
Loading
0