Skip to content

Theming fixes - #405

Merged
schiessle merged 4 commits into
masterfrom
theming-fixes
Jul 18, 2016
Merged

Theming fixes#405
schiessle merged 4 commits into
masterfrom
theming-fixes

Conversation

@nickvergessen

@nickvergessennickvergessen commented Jul 15, 2016

Copy link
Copy Markdown
Member

@nickvergessennickvergessen added this to the Nextcloud Next milestone Jul 15, 2016
@nickvergessennickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 15, 2016
@nickvergessennickvergessen mentioned this pull request Jul 15, 2016
2 tasks
@MorrisJobke

Copy link
Copy Markdown
Member

@nickvergessen Is this ready to review?


// setup realm
$defaults = new \OC_Defaults();
$defaults = new \OCP\Defaults();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reasons why we don't use \OC::$server->getThemingDefaults() here and below?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

getThemingDefaults() is marked as internal, and the constructor of OCP\Default uses this internal method after this PR.

@nickvergessennickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 18, 2016
@nickvergessen

Copy link
Copy Markdown
MemberAuthor

Yes, ready for review


public function getShortFooter() {
$slogan = $this->getSlogan();
$footer = '<a href="'. $this->getBaseUrl() . '" target="_blank"' .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

needs escaping?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No? Or atleast this is just copy paste of the parent class, so it would need fixing there as.

@MorrisJobke

Copy link
Copy Markdown
Member

Tested and works 👍 Code looks good

@schiessle

Copy link
Copy Markdown
Member

looks good 👍

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nickvergessen@MorrisJobke@schiessle@icewind1991