Split off from #919206: UI option is not saved.

Code comments were lifted from ... context_reaction_theme, but the one in Context.
Fixed indentation.
Cleaned up the foreach loops.

CommentFileSizeAuthor
#2 context_reaction_theme-code_cleanup-2.patch2.64 KBtim.plunkett
context_reaction_theme-code_cleanup.patch2.59 KBtim.plunkett

Comments

joachim’s picture

joachim commented

Looks good! Just a couple of points:

+++ context_reaction_theme.module	13 Oct 2010 17:29:55 -0000
@@ -48,4 +47,4 @@ function context_reaction_theme_context_
Index: plugins/context_reaction_active_theme.inc

+++ plugins/context_reaction_active_theme.inc	13 Oct 2010 17:29:55 -0000
@@ -31,17 +32,13 @@ class context_reaction_active_theme exte
+      if ($theme = $context->reactions[$this->plugin]['theme']) {

I think I'd prefer an isset() here first, *then* setting $theme to that value. Easier to follow what's going on IMO.

+++ plugins/context_reaction_active_theme.inc	13 Oct 2010 17:29:55 -0000
@@ -31,17 +32,13 @@ class context_reaction_active_theme exte
-  function execute(&$vars) {

I don't know enough about the context plugin system to know whether changing the signature of a method is ok or not...

Powered by Dreditor.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new context_reaction_theme-code_cleanup-2.patch2.64 KB

1) Switched it back to !empty().
2) An argument is only passed to execute() when it's needed. This was another remnant of copy/pasting another plugin as a starting point.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good to me :)