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

Improve Route Handling for Localized and Non-Localized Routes #1063

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

Mohtelsayed
Copy link
Contributor

This pull request addresses issue #963 .

Description:

This pull request enhances route handling to accommodate both localized and non-localized routes. The core changes include:

Key Changes:

Locale-Agnostic Route Comparison: Introduced removeLocaleFromRouteName to strip locale prefixes from route names. This allows for consistent route comparisons, regardless of the current locale.
Subpage Detection: Updated isCurrentRoutePathSubpageOf to consider locale-stripped route names. This ensures accurate subpage identification, even when switching between locales.
Route Inclusion Check: Modified currentRoutePathIncludes to utilize locale-stripped route names, improving the reliability of route inclusion checks.

Benefits:
Consistent Route Handling: Ensures that route comparisons and subpage detection work correctly across different locales.
Simplified Logic: Streamlines route handling by removing unnecessary locale-specific checks.
Improved User Experience: Provides a seamless user experience, especially when switching between languages or navigating complex route structures.
Testing:

Tested the changes in various scenarios, including:
Navigating to subpages within different locales
Checking route inclusion and subpage detection under different language settings
Please review the code changes and provide feedback.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for activist-org canceled.

Name Link
🔨 Latest commit f4b78ba
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/675ab398d5c9d30007a0d807

Copy link
Contributor

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The changelog has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

First PR Commit Check

  • The commit messages for the remote branch of a new contributor should be checked to make sure their email is set up correctly so that they receive credit for their contribution
    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo

@andrewtavis
Copy link
Member

Hi @Mohtelsayed 👋 FYI I'm seeing that your email isn't set up correctly for Git. Can you please check that the email you use for GitHub is the same as the response you get from git config user.email, and if not set it with git config --global user.email "YOUR_GITHUB_EMAIL". Without this your account won't get the contribution :)

@andrewtavis
Copy link
Member

Do you want to fix your email and then open another PR? You can also try to commit to this branch again :)

@Mohtelsayed
Copy link
Contributor Author

@andrewtavis Should be set up now. Let me know if there is anything else.

@andrewtavis
Copy link
Member

The commits here are still not for your account. Do you want to close this and open a new PR? :) Can also review this one, but as I said your account won't get the credit.

@Mohtelsayed
Copy link
Contributor Author

Ok sure

@Mohtelsayed Mohtelsayed reopened this Dec 12, 2024
@Mohtelsayed
Copy link
Contributor Author

@andrewtavis How about now. I think it should be set up. If not ill just make a new one.

@andrewtavis
Copy link
Member

I'll squash the commits and it should work now :) Thanks, @Mohtelsayed!

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution here, @Mohtelsayed! This definitely cleans up the route check functionality more and integrates well with the work from your classmates 😊

@andrewtavis andrewtavis merged commit 6871265 into activist-org:main Dec 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants