Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38133

html_writer: add shortcuts for very frequent tags

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Libraries

      Description

      The html_writer class is generally convenient and safe but can result in rather long code. For example, if you want to add some accesshide text:

      echo html_writer::tag('span', $str, array('class' => 'accesside'));
      

      The problem is exacerbated when you are creating a structure of multiple tags.

      I propose creating shortcut methods for div and span, which are the most common structural tags. The shortcuts should also make it easier to add a CSS class as this is usually required for structural tags. This will allow for shorter code. (Other tags can continue to use the current methods.)

      Proposed code:

      echo html_writer::span($str, 'accesside');
      

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              quen Sam Marshall added a comment -

              Requesting peer review.

              Note: I submitted this because when I whined about it being a pain to write out <div> tags in the developer chat, Petr said, why don't you submit a patch... so I am!

              Show
              quen Sam Marshall added a comment - Requesting peer review. Note: I submitted this because when I whined about it being a pain to write out <div> tags in the developer chat, Petr said, why don't you submit a patch... so I am!
              Hide
              quen Sam Marshall added a comment -

              Note: Just to demonstrate that they are indeed commonly used: in the complete Moodle core code, as of now, there are 1,040 uses of html_writer for div tags and 246 for span tags, all of which could be shortened using these new functions.

              Regex used for search (example for span tag):

              html_writer::(start_|end_)?tag\('span'

              Show
              quen Sam Marshall added a comment - Note: Just to demonstrate that they are indeed commonly used: in the complete Moodle core code, as of now, there are 1,040 uses of html_writer for div tags and 246 for span tags, all of which could be shortened using these new functions. Regex used for search (example for span tag): html_writer::(start_|end_)?tag\('span'
              Hide
              mudrd8mz David Mudrák added a comment -

              Very nice. Thanks a lot Sam.

              [Y] Syntax
              [Y] Output
              [Y] Whitespace
              [-] Language
              [-] Databases
              [-] Testing (haven't run unit tests but they are present and good quality)
              [-] Security
              [-] Documentation (please add dev_docs_required and api_change labels)
              [Y] Git
              [Y] Sanity check

              Show
              mudrd8mz David Mudrák added a comment - Very nice. Thanks a lot Sam. [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [-] Testing (haven't run unit tests but they are present and good quality) [-] Security [-] Documentation (please add dev_docs_required and api_change labels) [Y] Git [Y] Sanity check
              Hide
              salvetore Michael de Raadt added a comment -

              I'm neither here nor there on this one. It will shorten code a bit, but it will add to the complexity of the code.

              Show
              salvetore Michael de Raadt added a comment - I'm neither here nor there on this one. It will shorten code a bit, but it will add to the complexity of the code.
              Hide
              mudrd8mz David Mudrák added a comment -

              I don't think that shortening the code is the primary target here. The real problem is that if you try to be a good boy using output renderers, html_writer and CSS classes (as you should), producing a single <div> just sucks (and as theme designers demand, you should produce them a lot). This patch makes it suck less.

              Show
              mudrd8mz David Mudrák added a comment - I don't think that shortening the code is the primary target here. The real problem is that if you try to be a good boy using output renderers, html_writer and CSS classes (as you should), producing a single <div> just sucks (and as theme designers demand, you should produce them a lot). This patch makes it suck less.
              Hide
              quen Sam Marshall added a comment -

              Thanks for review David. Submitting for integration, let's see what they think.

              Btw, my opinion would be that this makes code simpler, not more complex.

              Show
              quen Sam Marshall added a comment - Thanks for review David. Submitting for integration, let's see what they think. Btw, my opinion would be that this makes code simpler, not more complex.
              Hide
              damyon Damyon Wiese added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              Thanks!

              Show
              damyon Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
              Hide
              timhunt Tim Hunt added a comment -

              These mostly look good, but I wonder if include_tag should also handle the case where $attributes already includes 'class'. It could add the new class name to that with a space, rather than overwriting it. Also, the function might better be called add_class.

              Show
              timhunt Tim Hunt added a comment - These mostly look good, but I wonder if include_tag should also handle the case where $attributes already includes 'class'. It could add the new class name to that with a space, rather than overwriting it. Also, the function might better be called add_class.
              Hide
              damyon Damyon Wiese added a comment -

              Actually - I dislike this change.

              The proper way to write a div is with a renderer->container method so it can be overridden with a theme. There are few exceptions mostly in old code - or renderers themselves.

              Adding this to html_writer implies that we want people to use html_writer::div everywhere.

              Show
              damyon Damyon Wiese added a comment - Actually - I dislike this change. The proper way to write a div is with a renderer->container method so it can be overridden with a theme. There are few exceptions mostly in old code - or renderers themselves. Adding this to html_writer implies that we want people to use html_writer::div everywhere.
              Hide
              poltawski Dan Poltawski added a comment -

              I don't really agree with Damyon, especially as its really really common for a div to be used for something much smaller than a proper container method would imply. We already have thousands of them.

              But, I think Tim's suggestion is a good one. (or at least warning the developer that their attribute is being ignored). I also think it would be worth starting a forum discussion about this, as there clearly are differing views.

              Show
              poltawski Dan Poltawski added a comment - I don't really agree with Damyon, especially as its really really common for a div to be used for something much smaller than a proper container method would imply. We already have thousands of them. But, I think Tim's suggestion is a good one. (or at least warning the developer that their attribute is being ignored). I also think it would be worth starting a forum discussion about this, as there clearly are differing views.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              mudrd8mz David Mudrák added a comment -

              I must slightly disagree with Damyon. I see the div elements as something that renderers use to build bigger parts of the page - such as a post forum post, submission, continue button etc. Themes may want to override these bigger blocks, change their order, inject something between them etc. But I have never seen a reasonable use case for actually replacing <div> produced by the container() with something else. Using the container() method has some advantages. But that does not make html_writer::tag('div') obsolete in any way. Note that when container() was implemented, we did not have the html_writer yet.

              And yes, Tim raised good point, as Dan noted.

              Show
              mudrd8mz David Mudrák added a comment - I must slightly disagree with Damyon. I see the div elements as something that renderers use to build bigger parts of the page - such as a post forum post, submission, continue button etc. Themes may want to override these bigger blocks, change their order, inject something between them etc. But I have never seen a reasonable use case for actually replacing <div> produced by the container() with something else. Using the container() method has some advantages. But that does not make html_writer::tag('div') obsolete in any way. Note that when container() was implemented, we did not have the html_writer yet. And yes, Tim raised good point, as Dan noted.
              Hide
              quen Sam Marshall added a comment -

              Dan: I've made a forum post for this

              https://moodle.org/mod/forum/discuss.php?d=223092

              I included in that post some of the arguments from this issue while simultaneously shooting them down, which is a bit unfair, but let's see how it goes.

              Tim/etc: I've changed code as suggested. (Waiting on phpunit init so I can actually run the new tests, will push again if I broke it.)

              Show
              quen Sam Marshall added a comment - Dan: I've made a forum post for this https://moodle.org/mod/forum/discuss.php?d=223092 I included in that post some of the arguments from this issue while simultaneously shooting them down, which is a bit unfair, but let's see how it goes. Tim/etc: I've changed code as suggested. (Waiting on phpunit init so I can actually run the new tests, will push again if I broke it.)
              Hide
              quen Sam Marshall added a comment -

              (I did break it, but have pushed working version now.)

              Show
              quen Sam Marshall added a comment - (I did break it, but have pushed working version now.)
              Hide
              damyon Damyon Wiese added a comment -

              OK - I have been convinced that there are enough fair uses of this in the renderers to make this worthwhile.

              Show
              damyon Damyon Wiese added a comment - OK - I have been convinced that there are enough fair uses of this in the renderers to make this worthwhile.
              Hide
              quen Sam Marshall added a comment -

              Thanks Damyon, all. Resubmitting for integration next time - I got a couple more positive reponses in the forum discussion, nobody was against.

              Show
              quen Sam Marshall added a comment - Thanks Damyon, all. Resubmitting for integration next time - I got a couple more positive reponses in the forum discussion, nobody was against.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

              Note this requires dev documentation, api changes, don't forget it.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! Note this requires dev documentation, api changes, don't forget it. Ciao
              Hide
              quen Sam Marshall added a comment -

              I wrote dev documentation

              http://docs.moodle.org/dev/html_writer#div.2C_span_methods

              Not sure if this needs linking from somewhere

              Show
              quen Sam Marshall added a comment - I wrote dev documentation http://docs.moodle.org/dev/html_writer#div.2C_span_methods Not sure if this needs linking from somewhere
              Hide
              dmonllao David Monllaó added a comment -

              Passing as running in the CI server

              Show
              dmonllao David Monllaó added a comment - Passing as running in the CI server
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

              Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

              Thanks, closing as fixed!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
              Hide
              bawjaws David Scotson added a comment -

              Added a link to a couple of similar changes.

              Show
              bawjaws David Scotson added a comment - Added a link to a couple of similar changes.

                People

                • Votes:
                  3 Vote for this issue
                  Watchers:
                  6 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/May/13