Skip to content

Remove duplicate IDs from customer account module links in Hummingbird#1033

Open
Codencode wants to merge 3 commits into
PrestaShop:2.xfrom
Codencode:fix-1032-hummingbird-displaycustomeraccount-link-scope
Open

Remove duplicate IDs from customer account module links in Hummingbird#1033
Codencode wants to merge 3 commits into
PrestaShop:2.xfrom
Codencode:fix-1032-hummingbird-displaycustomeraccount-link-scope

Conversation

@Codencode
Copy link
Copy Markdown
Contributor

@Codencode Codencode commented May 29, 2026

Questions Answers
Description? This PR removes static HTML ids from Hummingbird module account-link templates rendered through displayCustomerAccount. Since the same hook output can appear in both the account sidebar and the main My account page, static ids may be duplicated in the DOM. Removing those ids avoids invalid duplicated markup without requiring coordinated changes across modules.
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #1032
Sponsor company Codencode snc
How to test?
Related PR PrestaShop/ps_emailalerts#159

Notes

This PR is part of a two-step fix. It introduces the account rendering context in Hummingbird by passing isMainAccount to the displayCustomerAccount hook. A companion PR in ps_emailalerts consumes that parameter to generate distinct link IDs and complete the fix.

@Codencode Codencode added this to the v2.1.0 milestone May 29, 2026
@Codencode Codencode requested review from ga-devfront and tblivet May 29, 2026 13:49
@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard May 29, 2026
@Codencode Codencode changed the base branch from develop to 2.x May 29, 2026 13:49
@Hlavtox
Copy link
Copy Markdown
Contributor

Hlavtox commented May 29, 2026

I would just delete the id and call it a day. There can be more than 2 links - header, my account, footer, what ID will you use then? :-)

@Codencode
Copy link
Copy Markdown
Contributor Author

@Hlavtox
Yes, actually deleting the ID would be the best choice. I didn't do it because I thought it was necessary, but it's fine with me. That way there's no need to add the parameter to the hook. Shall I proceed this way?

@Codencode Codencode changed the title Pass isMainAccount to Hummingbird displayCustomerAccount hook Remove duplicate IDs from customer account module links in Hummingbird May 30, 2026
@Codencode
Copy link
Copy Markdown
Contributor Author

@Hlavtox
Applied your suggestion: I removed the static IDs from the Hummingbird module account links to avoid duplicate IDs without adding context-specific naming.

Copy link
Copy Markdown
Contributor

@tblivet tblivet left a comment

Choose a reason for hiding this comment

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

Hi @Codencode, I’m in favor of removing these IDs, but this would certainly break UI tests based on what I see here: https://github.com/PrestaShop/ui-testing-library/blob/main/src/versions/develop/pages/FO/hummingbird/myAccount/index.ts

What do you think @PrestaShop/qa-automation?

Hlavtox
Hlavtox previously approved these changes Jun 1, 2026
@ps-jarvis ps-jarvis added the Waiting for QA Status: Action required, Waiting for test feedback label Jun 1, 2026
@ps-jarvis ps-jarvis moved this from Ready for review to To be tested in PR Dashboard Jun 1, 2026
@Codencode
Copy link
Copy Markdown
Contributor Author

Hi @Codencode, I’m in favor of removing these IDs, but this would certainly break UI tests based on what I see here: https://github.com/PrestaShop/ui-testing-library/blob/main/src/versions/develop/pages/FO/hummingbird/myAccount/index.ts

What do you think @PrestaShop/qa-automation?

@tblivet good catch!

Maybe we could solve this by moving the current ID values to the class attribute instead?

This would still remove the duplicated IDs from the DOM, but it would keep a stable selector that could be used by the UI tests, for example by updating selectors from .account-menu__nav #wishlist_link to account-menu__nav .wishlist_link`.

What do you think?

@Codencode Codencode added Accessibility Screen readers, keyboard navigation, ARIA, contrast... and removed Waiting for QA Status: Action required, Waiting for test feedback labels Jun 1, 2026
@tblivet
Copy link
Copy Markdown
Contributor

tblivet commented Jun 2, 2026

@Codencode We have some elements that use specific classes in Hummingbird and are used in UI tests. They are defined as CSS modifiers, like account-menu__link--wishlist, so I think this is the best approach to maintain consistency.

@Codencode
Copy link
Copy Markdown
Contributor Author

@Codencode We have some elements that use specific classes in Hummingbird and are used in UI tests. They are defined as CSS modifiers, like account-menu__link--wishlist, so I think this is the best approach to maintain consistency.

@tblivet These classes already exist, so the PR is fine, meaning we just need to remove the IDs, right?

@tblivet
Copy link
Copy Markdown
Contributor

tblivet commented Jun 2, 2026

No, we don’t have it for now. We have account-menu__link--active, but we don’t have account-menu__link--wishlist.

@Codencode
Copy link
Copy Markdown
Contributor Author

@tblivet, if I understood your suggestion correctly, I added dedicated classes to these module account links so we can keep stable selectors without relying on duplicated IDs.

Copy link
Copy Markdown
Contributor

@tblivet tblivet left a comment

Choose a reason for hiding this comment

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

@Codencode Yes, that’s exactly what I meant 👍 We just need @PrestaShop/qa-automation to update the UI tests.

@Codencode
Copy link
Copy Markdown
Contributor Author

Perfect!
Should the tests be updated after this PR is merged, or before?

@tblivet
Copy link
Copy Markdown
Contributor

tblivet commented Jun 2, 2026

Good question 🤷‍♂️ I’ll try to create a PR this afternoon if you want 👍

@tblivet
Copy link
Copy Markdown
Contributor

tblivet commented Jun 2, 2026

Here is the associated PR for UI tests: PrestaShop/ui-testing-library#991

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accessibility Screen readers, keyboard navigation, ARIA, contrast...

Projects

Status: To be tested

Development

Successfully merging this pull request may close these issues.

displayCustomerAccount module links can generate duplicate HTML ids between sidebar and main account page

4 participants