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

[FEATURE] Add argument with page uid for building language menu. #620

Merged
merged 1 commit into from
Jul 24, 2014

Conversation

danilovq
Copy link

This feature will be helpfull if you need to build language menu for another page, not for current only.

@danilobuerger
Copy link

Hey @danilovq

thanks for your PR, but it can't be accepted as it is at the moment. Please see https://github.com/FluidTYPO3/vhs/blob/development/Classes/ViewHelpers/Page/InfoViewHelper.php on how we generally handle a optional pageUid argument. Please amend and force push (no second commit) your PR to include this.

if (0 === $this->pageUid) {
$this->pageUid = $GLOBALS['TSFE']->id;
}

Choose a reason for hiding this comment

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

Please move that whole block to parseLanguageMenu, then you can use pageUid as a local variable in that method

Copy link
Author

Choose a reason for hiding this comment

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

@danilobuerger I made it as class field because I use it in method getLanguageUrl too.

Choose a reason for hiding this comment

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

ah i didnt see that. in that case make it a method please so its testable

@danilobuerger
Copy link

alright i am good with the commit, will test tomorrow except if someone merges first ;-)

@xf-
Copy link
Contributor

xf- commented Jun 26, 2014

CGL - Please use tabs instead of spaces

* @return integer
*/
protected function getPageUid() {
$pageUid = intval($this->arguments['pageUid']);
Copy link
Member

Choose a reason for hiding this comment

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

Please use $pageUid = (integer) $this->arguments['pageUid'] - we use control structures before functions whenever this is possible.

@bjo3rnf
Copy link
Contributor

bjo3rnf commented Jul 16, 2014

Hi @danilovq,

would you please take a look at the comments above and finish your PR so we can close this issue? If you have questions feel free to ask. Thanks in advance for contributing.

Cheers
Björn

@NamelessCoder
Copy link
Member

  1. CGL must be respected
  2. Merge- and duplicate commits must be removed by squashing these commits

@NamelessCoder
Copy link
Member

Good job Anton - thanks!

NamelessCoder added a commit that referenced this pull request Jul 24, 2014
[FEATURE] Add argument with page uid for building language menu.
@NamelessCoder NamelessCoder merged commit 65cbe85 into FluidTYPO3:development Jul 24, 2014
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.

5 participants