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 navigation via info.xml (#26785) #2822

Merged
merged 3 commits into from
Jan 27, 2017
Merged

Conversation

LukasReschke
Copy link
Member

  • Read navigation information from info.xml

  • Load files navigation elements from info.xml

  • Add comment about ignoring the exception

From owncloud/core#26785

* Read navigation information from info.xml

* Load files navigation elements from info.xml

* Add comment about ignoring the exception

Signed-off-by: Lukas Reschke <[email protected]>
@LukasReschke LukasReschke added 3. to review Waiting for reviews downstream labels Dec 22, 2016
@LukasReschke LukasReschke added this to the Nextcloud 12.0 milestone Dec 22, 2016
@mention-bot
Copy link

@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @bartv2 and @MorrisJobke to be potential reviewers.

<navigation>
<route>files.view.index</route>
<order>0</order>
</navigation>
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

What I dislike about this is it drops the name tag by default, uses firstCaseUpper(appname). I think it's a trap for apps which then will accidently not have this translated anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Also this looks like an app can only have one? Should we wrap it in a "navigations" so we can have muliple?
Or should those apps just use the programmatic way then instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

What I dislike about this is it drops the name tag by default, uses firstCaseUpper(appname). I think it's a trap for apps which then will accidently not have this translated anymore.

We should probably work on getting our CI scripts running then in a way that it supports the lang= tag as seen at http://nextcloudappstore.readthedocs.io/en/latest/developer.html#info-xml

Copy link
Member

Choose a reason for hiding this comment

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

Can also adjust the travis thing to translate <navigation><name> when its provided?

Copy link
Member

Choose a reason for hiding this comment

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

@MorrisJobke can we add this simply? Extracting <navigation><name> automatically to travis?

Copy link
Member

Choose a reason for hiding this comment

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

@MorrisJobke can we add this simply? Extracting automatically to travis?

Create me a ticket - I will look into extending it 😉

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="32" width="32" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/">
Copy link
Member

Choose a reason for hiding this comment

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

no viewbox no minification

Copy link
Member

Choose a reason for hiding this comment

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

Done

<navigation>
<route>files.view.index</route>
<order>0</order>
</navigation>
Copy link
Member

Choose a reason for hiding this comment

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

Also this looks like an app can only have one? Should we wrap it in a "navigations" so we can have muliple?
Or should those apps just use the programmatic way then instead?

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.

Tested and works 👍

Regarding multiple entries: I would keep the info.xml with one entry. For multiple entries the app needs to use the normal OCP API.

Signed-off-by: Joas Schilling <[email protected]>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I made the name non-optional.
I'm fine with this now and will create the translation issue.

@MorrisJobke
Copy link
Member

👍 (ignore CI -.- there seems to be an issue with the autoloader check - I guess they released a new version between Job #1 and #2 - so one failed and one succeeded)

I will merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants