Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

made app compliant #76

Merged
merged 2 commits into from
Nov 17, 2015
Merged

made app compliant #76

merged 2 commits into from
Nov 17, 2015

Conversation

v1r0x
Copy link
Contributor

@v1r0x v1r0x commented Oct 15, 2015

fixes #75

@v1r0x v1r0x mentioned this pull request Oct 15, 2015
@v1r0x
Copy link
Contributor Author

v1r0x commented Oct 21, 2015

@Henni @brantje @DJaeger @jancborchardt can we merge this?

@Henni
Copy link
Contributor

Henni commented Oct 21, 2015

I'll take a look.
It isn't completely trivial to review this, as these changes could break features in unexpected ways.

@brantje
Copy link
Contributor

brantje commented Oct 21, 2015

Code looks good 👍

@@ -29,5 +30,5 @@

// the title of your application. This will be used in the
// navigation or on the settings page of your app
'name' => \OC_L10N::get('maps')->t('Maps')
'name' => $l->t('Maps')
));
Copy link
Contributor

Choose a reason for hiding this comment

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

While we are on it, let's remove all the deprecated function calls:

[...]

\OC::$server->getNavigationManager()->add(array(
    // the string under which your app will be referenced in owncloud
    'id' => 'maps',

    // sorting weight for the navigation. The higher the number, the higher
    // will it be listed in the navigation
    'order' => 10,

    // the route that will be shown on startup
    'href' => \OC::$server->getURLGenerator()->linkToRoute('maps.page.index'),

    // the icon that will be shown in the navigation
    // this file needs to exist in img/
    'icon' => \OC::$server->getURLGenerator()->imagePath('maps', 'maps.svg'),

    // the title of your application. This will be used in the
    // navigation or on the settings page of your app
    'name' => \OC::$server->getL10N('maps')->t('Maps')
));

@Henni
Copy link
Contributor

Henni commented Oct 21, 2015

Looks good besides my comment above. 👍

@v1r0x
Copy link
Contributor Author

v1r0x commented Oct 21, 2015

Should we merge this and create a new PR or add this to this PR?

@Henni
Copy link
Contributor

Henni commented Oct 21, 2015

If you are able to fix it now, do it in this PR. Otherwise we'll create another PR.

@jancborchardt
Copy link
Contributor

@v1r0x can you fix the problem noted by @Henni and then rebase? Cause this needs a rebase anyway before we can merge. :)

@v1r0x
Copy link
Contributor Author

v1r0x commented Nov 16, 2015

Looks like I messed up the rebase...Any idea how to revert it?

@v1r0x
Copy link
Contributor Author

v1r0x commented Nov 16, 2015

Thanks to @PVince81 "I" fixed it ;)

Please review

@jancborchardt
Copy link
Contributor

@Henni everything addressed you had in mind? :)

@jancborchardt
Copy link
Contributor

Btw @v1r0x you are in the ownCloud organization, so it would be much easier if you work on branches in this repo instead of having a personal fork. :) That way all other ownCloud org folks (such as @Henni and me) can collaborate.

@Henni
Copy link
Contributor

Henni commented Nov 17, 2015

what @jancborchardt said...

Otherwise this PR looks good 👍

@v1r0x
Copy link
Contributor Author

v1r0x commented Nov 17, 2015

Will do it next time :)
Ok, I'll merge it then?

@Henni
Copy link
Contributor

Henni commented Nov 17, 2015

Feel free to do so :)

v1r0x added a commit that referenced this pull request Nov 17, 2015
@v1r0x v1r0x merged commit ad4cb2a into owncloud-archive:master Nov 17, 2015
@v1r0x v1r0x deleted the app-comllant branch November 17, 2015 19:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make app compliant
4 participants