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

language based region nav config #3425

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

vhargrave
Copy link
Contributor

@vhargrave vhargrave commented Jan 7, 2025

Resolves: MWPW-164320

Adds support to the region nav for sites that are language-based.
Based on this github discussion. - https://github.com/orgs/adobecom/discussions/3383

Test URLs:

Express (with language map). (Notice how the Portuguese link changes to /br or the Mexican one to /es)

Express (with language map on a sub page that does have localized versions of the page)

Express (with language map on a blog page that doesn't have localized version of the page)

CC (no changes since this is opt-in and cc has no language map)

Copy link
Contributor

aem-code-sync bot commented Jan 7, 2025

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link
Contributor

aem-code-sync bot commented Jan 7, 2025

Page Scores Audits Google
📱 /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 96.45%. Comparing base (82694a3) to head (485b443).
Report is 10 commits behind head on stage.

Files with missing lines Patch % Lines
libs/blocks/region-nav/region-nav.js 75.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #3425      +/-   ##
==========================================
- Coverage   96.46%   96.45%   -0.01%     
==========================================
  Files         255      255              
  Lines       59363    59378      +15     
==========================================
+ Hits        57266    57276      +10     
- Misses       2097     2102       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aem-code-sync aem-code-sync bot temporarily deployed to vhargrave/language-based-region-nav-config January 7, 2025 14:29 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to vhargrave/language-based-region-nav-config January 7, 2025 14:30 Inactive
@mokimo
Copy link
Contributor

mokimo commented Jan 7, 2025

Seems like the nala tests fail can you try

  1. to purge the code of ur branch? - example purge request: CURL -si -X POST https://admin.hlx.page/code/mokimo/milo/thebranchimworkingon/* and re-run the nala tests
  2. if that doesn't work, to rebase to stage & then push ur branch?

Copy link
Contributor

github-actions bot commented Jan 8, 2025

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@vhargrave
Copy link
Contributor Author

Seems like the nala tests fail can you try

  1. to purge the code of ur branch? - example purge request: CURL -si -X POST https://admin.hlx.page/code/mokimo/milo/thebranchimworkingon/* and re-run the nala tests
  2. if that doesn't work, to rebase to stage & then push ur branch?

@mokimo did that, but now the nala tests started getting skipped even though I didn't configure that. They're getting skipped for other PRs too. It's a little strange.

@mokimo
Copy link
Contributor

mokimo commented Jan 8, 2025

@vhargrave there's two nala suites running
https://github.com/adobecom/milo/actions/runs/12655761620/workflow?pr=3425 & https://github.com/adobecom/milo/actions/runs/12655761629/workflow?pr=3425

The skipped test conditionally runs on consumers if: contains(join(github.event.pull_request.labels.*.name, ' '), 'run-nala-on-') based on adding a label. Or at least thats what it seems like to me looking at the WF files.

Here's how you can quickly connect a check to it's workflow file

  1. Go into the check details
    image
  2. Check how the action is even triggered
    image

@vhargrave
Copy link
Contributor Author

@vhargrave there's two nala suites running https://github.com/adobecom/milo/actions/runs/12655761620/workflow?pr=3425 & https://github.com/adobecom/milo/actions/runs/12655761629/workflow?pr=3425

The skipped test conditionally runs on consumers if: contains(join(github.event.pull_request.labels.*.name, ' '), 'run-nala-on-') based on adding a label. Or at least thats what it seems like to me looking at the WF files.

Here's how you can quickly connect a check to it's workflow file

  1. Go into the check details
    image
  2. Check how the action is even triggered
    image

@mokimo Ah I see , thanks 👍. And I can see the other suite is running which is what we wanted 👍
image

Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

Cool 🚀

@vhargrave vhargrave added the high priority Why is this a high priority? Blocker? Critical? Dependency? label Jan 9, 2025
@hadobe hadobe self-requested a review January 9, 2025 09:15
@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jan 9, 2025

Skipped merging 3425: language based region nav config due to failing checks

@mokimo mokimo merged commit 5bbb0bf into stage Jan 9, 2025
23 of 24 checks passed
@mokimo mokimo deleted the vhargrave/language-based-region-nav-config branch January 9, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Why is this a high priority? Blocker? Critical? Dependency? Ready for Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants