[buddypress-trac] [BuddyPress Trac] #7157: UI to pick Template Packs

[buddypress-trac]

#7157: UI to pick Template Packs
------------------------------------+------------------
 Reporter:  DJPaul                  |       Owner:
     Type:  defect (bug)            |      Status:  new
 Priority:  strategic               |   Milestone:  2.7
Component:  Administration          |     Version:
 Severity:  normal                  |  Resolution:
 Keywords:  has-patch dev-feedback  |
------------------------------------+------------------

Comment (by DJPaul):

 `bp_core_admin_theme_packages_get_head`
  * The `array_fill_keys` strikes me as unnecessarily complex and harder to
 read, considering the content is static.
  * The PHPDoc indicates a `return` type, but this function prints text to
 output.
  * The various `printf` parameters need appropriate escaping. I know the
 various column names and values are static, but they should be escaped to
 make sure they will always be safe in future (i.e. if someone adds
 something and doesn't read the rest of the function correctly).

 `bp_core_admin_theme_package_metas`
  * The PHPDoc indicates a `return` type, but this function prints text to
 output.
  * Why is `$theme_package_id` an optional argument, if no default value is
 provided inside the function?

 `bp_core_admin_theme_package_get_warnings`
  * PHPDoc says it returns a string, but it actually returns an array.
  * "This component is often forgotten" -- as wikipedia says in English:
 citation needed. :) Where/how? Some reasoning for this assumption would be
 useful, thank you.

 `bp_core_admin_theme_packages_get_body`
  * The PHPDoc indicates a `return` type, but this function prints text to
 output.
  * On the line starting `<tr id="<?php echo esc_attr( $theme_package->id
 ); ?>"`, for the `class`, please concatenate the strings *then*
 `esc_attr`. Don't `esc_attr` twice and then glue them together.
  * Some inline CSS, can we move to one of the admin stylesheets?
  * Empty `span` in the `plugin-title` block. And `row-actions-visible`.
 Suspicious.
  * That `check-column` `th` is also suspiciously empty. Maybe copy what
 WordPress does -- I will be surprised if it should be left blank.
   * I imagine @mercime will be happy to help iterate on this form for
 accessibility guidance once the first version is committed, so don't worry
 too much yet.
  * On the line starting `<label for="bp_theme_package-`, please `esc_attr`
 for the entire `for` attribute value. i.e. move "bp_theme_package-" inside
 the `esc_attr` and concatenate it there.
  * I think the list of warning in the `attntion` `div` should not be split
 by `<br/>`. I think we had an accessibility change recently where we
 swapped this. They should each be `<p>`, or perhaps a `<ul>`.

 `bp_core_admin_theme_packages`
  * Some inline CSS, can we move to one of the admin stylesheets?

 Apart from all of this, I think the function names are too long (not your
 fault, but we don't have to copy what we've done before if we can do it
 better now).

 Please only use `_get_` in function names if they return a value. That's
 the pattern we try to keep to throughout the project.

 And, why did you decide to put all this new code in a new file? Is
 splitting the other admin UI screens into their own files an approach you
 think we should do (in the future)?

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/7157#comment:17>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac