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

Add query string to LangSwitcher #229

Closed
wants to merge 1 commit into from
Closed

Add query string to LangSwitcher #229

wants to merge 1 commit into from

Conversation

Antikon
Copy link
Contributor

@Antikon Antikon commented Oct 9, 2019

What are you changing/introducing

Add a simple function appendQueryString() which gets query string from current request and append it to the link

What is the reason for changing/introducing

Fix #220

QA

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Fixed issues #220

Copy link
Member

@nadar nadar left a comment

Choose a reason for hiding this comment

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

could you please add a unit test. also i think this will now append always a "?" even there are no params.

@Antikon
Copy link
Contributor Author

Antikon commented Oct 13, 2019

Sorry for such a silly mistake!

I need an advise on Unit Tests. I am not good enough in them.

Currently, the LangSwitcher Tests rely on data from cms_nav_item table in luya_env_phpunit db. The data refer to only one language (en). So in tests we always have a second link to a German homepage. I need to add some data in cms_nav_item for another language to test the LangSwitcher transitions between some internal pages (with and without query string).

What should I do?

@nadar
Copy link
Member

nadar commented Dec 18, 2019

In the next version of the core we have an append function:

https://github.com/luyadev/luya/blob/master/core/helpers/Url.php#L133-L169

Maybe we can use this one afterwards. Also we need a unit test.

in order to test only the lang switcher with sqlite, there is a test for that: https://github.com/luyadev/luya-module-cms/blob/master/tests/src/widgets/LangSwitcherSqliteTest.php

so you can just run: ./vendor/bin/phpunit tests/widgets/LangSwitcherSqliteTest.php to only test that file and don't care about the rest.

@nadar
Copy link
Member

nadar commented Oct 5, 2020

@Antikon Is there something i can help? or should we close this PR?

@nadar
Copy link
Member

nadar commented Feb 11, 2021

I am going to close this PR because:

  • The PR has no activity for a long time.
  • The PR does not have Unit Tests.
  • The PR is not valid.
  • The PR author does not respond to questions are comments.

If the author (you) or someone is picking up the PR again, feel free to reopen. We don't want to wast the time you have spent on the PR. Also if you need help please let us know. Its not our aim to close PR's but we want to keep the repository activity as clean as possible.

If you think this is still important or there are more/new informations. Please reopen the PR and let us know.

@nadar nadar closed this Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LangSwitcher should keep query string in URL (or at least have a such option)
2 participants