Skip to content

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Feb 22, 2019

Close #14052

Test

Pick any app with <navigations> in info.xml (e.g. activity) and add a second <navigation> item.

Expected

Two navigation items for this app

Actual

No navigation entry for this app

Known limitation

If id is not defined for navigation app name is used as fallback. At least one navigation item needs the id element with a value not equal to app name.

Signed-off-by: Daniel Kesselberg <[email protected]>
@kesselb kesselb force-pushed the bugfix/14052-multiple-navigation-items branch from f8a7b91 to 36c51bc Compare February 22, 2019 14:57
@nickvergessen
Copy link
Member

We could fallback to <appid><$i>

@kesselb
Copy link
Contributor Author

kesselb commented Feb 22, 2019

We could fallback to <appid><$i>

Don't have a strong opinion on that. Do you think someone would bind application logic to an navigation item and use <li data-id="activity">for it?

$id = isset($nav['id']) ? $nav['id'] : $app . ($navKey === 0 ? '' : $navKey);

Not nice but backwards compatible 🙈

$id = $nav['id'] ?? $app . ($navKey === 0 ? '' : $navKey);

PHP7+ looks a bit nicer 🤣

@nickvergessen
Copy link
Member

Good news, master is 7.1+
So I would go for that.

Well I think at least items should have a unique ID.

Signed-off-by: Daniel Kesselberg <[email protected]>
Signed-off-by: Daniel Kesselberg <[email protected]>
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good, tested and works 👍

@juliusknorr
Copy link
Member

Failures unrelated.

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 27, 2019
@MorrisJobke MorrisJobke merged commit e5cacc0 into master Feb 27, 2019
@MorrisJobke MorrisJobke deleted the bugfix/14052-multiple-navigation-items branch February 27, 2019 15:08
@MorrisJobke
Copy link
Member

/backport to stable15

@MorrisJobke
Copy link
Member

/backport to stable14

@backportbot-nextcloud
Copy link

backport to stable15 in #14416

@backportbot-nextcloud
Copy link

backport to stable14 in #14417

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

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants