Moodle
  1. Moodle
  2. MDL-22937

Message styles need consolidation across chat/messaging/comments

    Details

    • Testing Instructions:
      Hide

      You'll need 2 users and 2 browsers (Firefox and Chrome for example). We're mostly just testing to make sure that nothing got messed up.

      As a teacher or admin create a chat activity.
      Go into the chatroom.
      In your other browser log in as your 2nd user and enter the chatroom.
      Check that both users can participate and that it all displays reasonably.

      Near the bottom of the chat window is "Themes >>". Switch one (or both users to bubble)
      Comment as each user and check that it still displays fine.

      Go to a course page and add a comment block.
      Comment and check that the display of comments seems ok.

      Go to messaging and message another user.
      Check that the display of your previous messages seems ok.

      Show
      You'll need 2 users and 2 browsers (Firefox and Chrome for example). We're mostly just testing to make sure that nothing got messed up. As a teacher or admin create a chat activity. Go into the chatroom. In your other browser log in as your 2nd user and enter the chatroom. Check that both users can participate and that it all displays reasonably. Near the bottom of the chat window is "Themes >>". Switch one (or both users to bubble) Comment as each user and check that it still displays fine. Go to a course page and add a comment block. Comment and check that the display of comments seems ok. Go to messaging and message another user. Check that the display of your previous messages seems ok.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-22937_message_html2
    • Rank:
      273

      Description

      The HTML and CSS for these things is completely different in each case:

      • messages
      • comments
      • chat AJAX bubbles version
      • chat AJAX compact version
      • chat old Javascript version
      • chat accessible version

      We need to look at these in detail next week and make them have the same HTML structure and CSS classes, so that themers have some chance to theme these to look similar.

      At very least we can change them all independently, but ideally a single renderer might be the best.

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          Here is the Chat HTML for reference.

          Accessible chat: descriptive list, one item per message

          <div id="messages">
          <h2>Messages</h2>
          <dl><dt class="title">13:12 Admin:</dt><dd class="text">hello again</dd></dl>
          <dl><dt class="title">13:11 Admin:</dt><dd class="text">hello</dd></dl>
          </div>

          AXAX compact: unsorted list, one item per message

          <div class="box " id="chat-messages">
          <ul id="messages-list">
          <li id="mdl-chat-entry-4" class="mdl-chat-my-entry">
          <div class="chat-message">
          <div class="meta">
          <span class="time">13:14</span>
          <span class="user"><a target="_blank" href="http://martin.moodle.local/head/user/view.php?id=2&course=2">Admin User</a></span>
          </div>
          <div class="text">hello</div>
          </div>
          </li>
          <li id="mdl-chat-entry-5" class="mdl-chat-my-entry">
          <div class="chat-message">
          <div class="meta">
          <span class="time">13:14</span>
          <span class="user"><a target="_blank" href="http://martin.moodle.local/head/user/view.php?id=2&course=2">Admin User</a></span>
          </div>
          <div class="text">hello again</div>
          </div>
          </li>
          </ul>
          </div>

          AJAX bubble: unsorted list containing table containing table, one list item per message

          <div class="box " id="chat-messages">
          <ul id="messages-list"><li id="mdl-chat-entry-6" class="mdl-chat-my-entry">
          <table align="right" class="chat-message"><tbody><tr>
          <td class="text">
          <table cellspacing="0" cellpadding="0" border="0" class="mymessage">
          <tbody>
          <tr><td class="topleft"/><td class="top"/><td class="topright"/></tr>
          <tr>
          <td class="left"/>
          <td class="conmts">
          hello
          </td>
          <td class="right"/>
          </tr>
          <tr>
          <td class="bottomleft"/>
          <td class="bottom"/>
          <td class="bottomright"/>
          </tr>
          </tbody>
          </table>
          </td>
          <td width="32px" valign="middle" class="picture">
          <a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" target="_blank"/><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a>
          </td>
          </tr><tr>
          <td align="right" colspan="2">
          <span class="time">13:17</span>
          <span class="user">Admin User</span>
          </td>
          </tr></tbody></table>
          </li><li id="mdl-chat-entry-7" class="mdl-chat-my-entry">

          <table align="right" class="chat-message"><tbody><tr>
          <td class="text">
          <table cellspacing="0" cellpadding="0" border="0" class="mymessage">
          <tbody>
          <tr><td class="topleft"/><td class="top"/><td class="topright"/></tr>
          <tr>
          <td class="left"/>
          <td class="conmts">
          hello again
          </td>
          <td class="right"/>
          </tr>
          <tr>
          <td class="bottomleft"/>
          <td class="bottom"/>
          <td class="bottomright"/>
          </tr>
          </tbody>
          </table>
          </td>
          <td width="32px" valign="middle" class="picture">
          <a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" target="_blank"/><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a>
          </td>
          </tr><tr>
          <td align="right" colspan="2">
          <span class="time">13:17</span>
          <span class="user">Admin User</span>
          </td>
          </tr></tbody></table></li></ul></div>

          Old Javascript version: one table per message

          <table class="chat-message">
          <tbody>
          <tr class="r1">
          <td valign="top" class="picture"><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" onclick="window.open('http://martin.moodle.local/head/user/view.php?id=2&course=2')"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a></td>
          <td class="text"><span class="title">13:22 Admin</span>: hello </td>
          </tr>
          </tbody>
          </table>

          <table class="chat-message">
          <tbody>
          <tr class="r1">
          <td valign="top" class="picture"><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" onclick="window.open('http://martin.moodle.local/head/user/view.php?id=2&course=2')"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a></td>
          <td class="text"><span class="title">13:23 Admin</span>: hello again</td>
          </tr>
          </tbody>
          </table>

          Show
          Martin Dougiamas added a comment - Here is the Chat HTML for reference. Accessible chat: descriptive list, one item per message <div id="messages"> <h2>Messages</h2> <dl><dt class="title">13:12 Admin:</dt><dd class="text">hello again</dd></dl> <dl><dt class="title">13:11 Admin:</dt><dd class="text">hello</dd></dl> </div> AXAX compact: unsorted list, one item per message <div class="box " id="chat-messages"> <ul id="messages-list"> <li id="mdl-chat-entry-4" class="mdl-chat-my-entry"> <div class="chat-message"> <div class="meta"> <span class="time">13:14</span> <span class="user"><a target="_blank" href="http://martin.moodle.local/head/user/view.php?id=2&course=2">Admin User</a></span> </div> <div class="text">hello</div> </div> </li> <li id="mdl-chat-entry-5" class="mdl-chat-my-entry"> <div class="chat-message"> <div class="meta"> <span class="time">13:14</span> <span class="user"><a target="_blank" href="http://martin.moodle.local/head/user/view.php?id=2&course=2">Admin User</a></span> </div> <div class="text">hello again</div> </div> </li> </ul> </div> AJAX bubble: unsorted list containing table containing table, one list item per message <div class="box " id="chat-messages"> <ul id="messages-list"><li id="mdl-chat-entry-6" class="mdl-chat-my-entry"> <table align="right" class="chat-message"><tbody><tr> <td class="text"> <table cellspacing="0" cellpadding="0" border="0" class="mymessage"> <tbody> <tr><td class="topleft"/><td class="top"/><td class="topright"/></tr> <tr> <td class="left"/> <td class="conmts"> hello </td> <td class="right"/> </tr> <tr> <td class="bottomleft"/> <td class="bottom"/> <td class="bottomright"/> </tr> </tbody> </table> </td> <td width="32px" valign="middle" class="picture"> <a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" target="_blank"/><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a> </td> </tr><tr> <td align="right" colspan="2"> <span class="time">13:17</span> <span class="user">Admin User</span> </td> </tr></tbody></table> </li><li id="mdl-chat-entry-7" class="mdl-chat-my-entry"> <table align="right" class="chat-message"><tbody><tr> <td class="text"> <table cellspacing="0" cellpadding="0" border="0" class="mymessage"> <tbody> <tr><td class="topleft"/><td class="top"/><td class="topright"/></tr> <tr> <td class="left"/> <td class="conmts"> hello again </td> <td class="right"/> </tr> <tr> <td class="bottomleft"/> <td class="bottom"/> <td class="bottomright"/> </tr> </tbody> </table> </td> <td width="32px" valign="middle" class="picture"> <a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" target="_blank"/><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a> </td> </tr><tr> <td align="right" colspan="2"> <span class="time">13:17</span> <span class="user">Admin User</span> </td> </tr></tbody></table></li></ul></div> Old Javascript version: one table per message <table class="chat-message"> <tbody> <tr class="r1"> <td valign="top" class="picture"><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" onclick="window.open('http://martin.moodle.local/head/user/view.php?id=2&course=2')"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a></td> <td class="text"><span class="title">13:22 Admin</span>: hello </td> </tr> </tbody> </table> <table class="chat-message"> <tbody> <tr class="r1"> <td valign="top" class="picture"><a href="http://martin.moodle.local/head/user/view.php?id=2&course=2" onclick="window.open('http://martin.moodle.local/head/user/view.php?id=2&course=2')"><img width="35" height="35" class="userpicture defaultuserpic" alt="Picture of Admin User" src="http://martin.moodle.local/head/theme/image.php?theme=standard&image=u%2Ff2&rev=164"/></a></td> <td class="text"><span class="title">13:23 Admin</span>: hello again</td> </tr> </tbody> </table>
          Hide
          Martin Dougiamas added a comment -

          I think the final solution should probably be just divs (no tables, spans or lists)

          Show
          Martin Dougiamas added a comment - I think the final solution should probably be just divs (no tables, spans or lists)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.

          Show
          Eloy Lafuente (stronk7) added a comment - NOTE: This issue was assigned to the STABLE backlog without complete triaging process. Marking it as triaged, but with this note for future reference.
          Hide
          Andrew Davis added a comment - - edited

          I am attempting to estimate my future work so that I can manage my time better in future. My current estimate for this issue is 7 hours to get this issue through development, integration and testing.

          update: Im intending to spend around 4 hours investigating and opening subtasks.

          Show
          Andrew Davis added a comment - - edited I am attempting to estimate my future work so that I can manage my time better in future. My current estimate for this issue is 7 hours to get this issue through development, integration and testing. update: Im intending to spend around 4 hours investigating and opening subtasks.
          Hide
          Andrew Davis added a comment -

          I've been through these various places and have taken a look at the html produced and where it comes from.

          This is a message from the current user to another user. Its produced by message_print_message_history(). No tables. Just spans and divs.

          <div class="mdl-left left ">
          <div class="message me">
          <a name="m59"></a>
          <span class="time">11:47 AM</span>
          :
          <span class="content">me again</span>
          </div>
          </div>
          

          Here is the html for the comment block. No tables. There is a UL and divs. The html is produced by comment::output() using a template that is defined in the constructor.

          <ul id="comment-list-4ffa4848a6c56" class="comment-list comments-loaded">
          <li class="first"></li>
          <li id="comment-1-4ffa4848a6c56">
          <div class="comment-userpicture">
          <div class="comment-content">
          <a href="http://localhost/moodle/dev/master/user/view.php?id=2&course=2">Admin User</a>
          -
          <span>9 Jul, 10:56</span>
          <div>
          </div>
          </li>
          </ul>
          

          Here is the html for chat (compact). Again, we have a list, an LI this time, and some divs. The template is defined in mod/chat/gui_ajax/theme/compact/config.php

          <li id="mdl-chat-entry-4" class="mdl-chat-my-entry"><div class="chat-message">
              <div class="meta">
                  <span class="time">10:46</span>
                  <span class="user"><a target="_blank" href="http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2">Admin User</a></span>
              </div>
              <div class="text">
              blamo
              </div>
          </div>
          </li>
          

          Here is the html for chat (bubble). A UL and a few tables. The template is defined in mod/chat/gui_ajax/theme/bubble/config.php

          <ul id="messages-list"><li id="mdl-chat-entry-3" class="mdl-chat-my-entry"><table align="right" class="chat-message"><tbody><tr>
          <td class="text">
              <table cellspacing="0" cellpadding="0" border="0" class="mymessage">
              <tbody>
                  <tr><td class="topleft"></td><td class="top"></td><td class="topright"></td></tr>
                  <tr>
                      <td class="left"></td>
                      <td class="conmts">
                      blam
                      </td>
                      <td class="right"></td>
                  </tr>
                  <tr>
                      <td class="bottomleft"></td>
                      <td class="bottom"></td>
                      <td class="bottomright"></td>
                  </tr>
              </tbody>
              </table>
          </td>
          <td width="32px" valign="middle" class="picture">
          <a href="http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" target="_blank"></a><a href="http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2"><img width="35" height="35" class="userpicture defaultuserpic" title="Picture of Admin User" alt="Picture of Admin User" src="http://localhost/moodle/dev/master/theme/image.php/standard/core/1341794778/u/f2"></a>
          </td>
          </tr><tr>
          <td align="right" colspan="2">
              <span class="time">10:44</span>
              <span class="user">Admin User</span>
          </td>
          </tr></tbody></table></li><li id="mdl-chat-entry-4" class="mdl-chat-my-entry"><table align="right" class="chat-message"><tbody><tr>
          <td class="text">
              <table cellspacing="0" cellpadding="0" border="0" class="mymessage">
              <tbody>
                  <tr><td class="topleft"></td><td class="top"></td><td class="topright"></td></tr>
                  <tr>
                      <td class="left"></td>
                      <td class="conmts">
                      blamo
                      </td>
                      <td class="right"></td>
                  </tr>
                  <tr>
                      <td class="bottomleft"></td>
                      <td class="bottom"></td>
                      <td class="bottomright"></td>
                  </tr>
              </tbody>
              </table>
          </td>
          <td width="32px" valign="middle" class="picture">
          <a href="http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" target="_blank"></a><a href="http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2"><img width="35" height="35" class="userpicture defaultuserpic" title="Picture of Admin User" alt="Picture of Admin User" src="http://localhost/moodle/dev/master/theme/image.php/standard/core/1341794778/u/f2"></a>
          </td>
          </tr><tr>
          <td align="right" colspan="2">
              <span class="time">10:46</span>
              <span class="user">Admin User</span>
          </td>
          </tr></tbody></table></li></ul>
          

          Here is the html for non-ajax chat. It's html is very simple although it uses the rather obscure DL (definition list). The html is produced by chat_format_message_manually() in mod/chat/lib.php

          <dl><dt class="title">11:19 Admin:</dt><dd class="text">working?</dd></dl>
          
          Show
          Andrew Davis added a comment - I've been through these various places and have taken a look at the html produced and where it comes from. This is a message from the current user to another user. Its produced by message_print_message_history(). No tables. Just spans and divs. <div class= "mdl-left left " > <div class= "message me" > <a name= "m59" ></a> <span class= "time" >11:47 AM</span> : <span class= "content" >me again</span> </div> </div> Here is the html for the comment block. No tables. There is a UL and divs. The html is produced by comment::output() using a template that is defined in the constructor. <ul id= "comment-list-4ffa4848a6c56" class= "comment-list comments-loaded" > <li class= "first" ></li> <li id= "comment-1-4ffa4848a6c56" > <div class= "comment-userpicture" > <div class= "comment-content" > <a href= "http: //localhost/moodle/dev/master/user/view.php?id=2&course=2" >Admin User</a> - <span>9 Jul, 10:56</span> <div> </div> </li> </ul> Here is the html for chat (compact). Again, we have a list, an LI this time, and some divs. The template is defined in mod/chat/gui_ajax/theme/compact/config.php <li id= "mdl-chat-entry-4" class= "mdl-chat-my-entry" ><div class= "chat-message" > <div class= "meta" > <span class= "time" >10:46</span> <span class= "user" ><a target= "_blank" href= "http: //localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" >Admin User</a></span> </div> <div class= "text" > blamo </div> </div> </li> Here is the html for chat (bubble). A UL and a few tables. The template is defined in mod/chat/gui_ajax/theme/bubble/config.php <ul id= "messages-list" ><li id= "mdl-chat-entry-3" class= "mdl-chat-my-entry" ><table align= "right" class= "chat-message" ><tbody><tr> <td class= "text" > <table cellspacing= "0" cellpadding= "0" border= "0" class= "mymessage" > <tbody> <tr><td class= "topleft" ></td><td class= "top" ></td><td class= "topright" ></td></tr> <tr> <td class= "left" ></td> <td class= "conmts" > blam </td> <td class= "right" ></td> </tr> <tr> <td class= "bottomleft" ></td> <td class= "bottom" ></td> <td class= "bottomright" ></td> </tr> </tbody> </table> </td> <td width= "32px" valign= "middle" class= "picture" > <a href= "http: //localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" target= "_blank" ></a><a href= "http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" ><img width= "35" height= "35" class= "userpicture defaultuserpic" title= "Picture of Admin User" alt= "Picture of Admin User" src= "http://localhost/moodle/dev/master/theme/image.php/standard/core/1341794778/u/f2" ></a> </td> </tr><tr> <td align= "right" colspan= "2" > <span class= "time" >10:44</span> <span class= "user" >Admin User</span> </td> </tr></tbody></table></li><li id= "mdl-chat-entry-4" class= "mdl-chat-my-entry" ><table align= "right" class= "chat-message" ><tbody><tr> <td class= "text" > <table cellspacing= "0" cellpadding= "0" border= "0" class= "mymessage" > <tbody> <tr><td class= "topleft" ></td><td class= "top" ></td><td class= "topright" ></td></tr> <tr> <td class= "left" ></td> <td class= "conmts" > blamo </td> <td class= "right" ></td> </tr> <tr> <td class= "bottomleft" ></td> <td class= "bottom" ></td> <td class= "bottomright" ></td> </tr> </tbody> </table> </td> <td width= "32px" valign= "middle" class= "picture" > <a href= "http: //localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" target= "_blank" ></a><a href= "http://localhost/moodle/dev/master/user/view.php?id=2&amp;course=2" ><img width= "35" height= "35" class= "userpicture defaultuserpic" title= "Picture of Admin User" alt= "Picture of Admin User" src= "http://localhost/moodle/dev/master/theme/image.php/standard/core/1341794778/u/f2" ></a> </td> </tr><tr> <td align= "right" colspan= "2" > <span class= "time" >10:46</span> <span class= "user" >Admin User</span> </td> </tr></tbody></table></li></ul> Here is the html for non-ajax chat. It's html is very simple although it uses the rather obscure DL (definition list). The html is produced by chat_format_message_manually() in mod/chat/lib.php <dl><dt class= "title" >11:19 Admin:</dt><dd class= "text" >working?</dd></dl>
          Hide
          Andrew Davis added a comment - - edited

          These structures can certainly be normalized somewhat.

          Shifting them into a common renderer would also be possible although the transition may be quite painful. Visual differences like the left/right positioning of messages within the messaging system Vs the down the left positioning in the non-ajax chat should be do-able with css. We'll just have to add descriptive class names to every element. That said, I'm not sure how beneficial total consolidation would be.

          If we are to switch to a common format we'll unfortunately pretty much need to do it all simultaneously. We can't be 100% certain our common format works until we've seen it used in each place.

          As a first step I'd suggest figuring out how to get the bubble format chat to work with only divs (and maybe a UL or LI). Once everything is div based it'll be easier to figure out if a common format is viable.

          Show
          Andrew Davis added a comment - - edited These structures can certainly be normalized somewhat. Shifting them into a common renderer would also be possible although the transition may be quite painful. Visual differences like the left/right positioning of messages within the messaging system Vs the down the left positioning in the non-ajax chat should be do-able with css. We'll just have to add descriptive class names to every element. That said, I'm not sure how beneficial total consolidation would be. If we are to switch to a common format we'll unfortunately pretty much need to do it all simultaneously. We can't be 100% certain our common format works until we've seen it used in each place. As a first step I'd suggest figuring out how to get the bubble format chat to work with only divs (and maybe a UL or LI). Once everything is div based it'll be easier to figure out if a common format is viable.
          Hide
          Martin Dougiamas added a comment -

          Nice to see you're working on this, thanks Andrew (and good luck!)

          Show
          Martin Dougiamas added a comment - Nice to see you're working on this, thanks Andrew (and good luck!)
          Hide
          Andrew Davis added a comment -

          Heres is the beginnings of a possible solution for div/span + css based bubble chat. I'm just wanting to get this direction validated before I spend too much more time on this. If this approach is ok there is a bunch more cleanup required including deleting the images used by the tables based version.

          I've added a new css file that I did not write. Its from http://nicolasgallagher.com/pure-css-speech-bubbles/ and has been dual licensed under MIT and GNU GPLv2.

          Show
          Andrew Davis added a comment - Heres is the beginnings of a possible solution for div/span + css based bubble chat. I'm just wanting to get this direction validated before I spend too much more time on this. If this approach is ok there is a bunch more cleanup required including deleting the images used by the tables based version. I've added a new css file that I did not write. Its from http://nicolasgallagher.com/pure-css-speech-bubbles/ and has been dual licensed under MIT and GNU GPLv2.
          Hide
          Andrew Davis added a comment -

          Ive pinged Sam and Eloy to get their thoughts on pulling in an external css file and where I have put it.

          Show
          Andrew Davis added a comment - Ive pinged Sam and Eloy to get their thoughts on pulling in an external css file and where I have put it.
          Hide
          Martin Dougiamas added a comment -

          Have not looked at your patch yet but those examples look awesome. And MIT license is compatible with us.

          Testing will have to make sure it looks good on all browsers we support.

          Show
          Martin Dougiamas added a comment - Have not looked at your patch yet but those examples look awesome. And MIT license is compatible with us. Testing will have to make sure it looks good on all browsers we support.
          Hide
          Andrew Davis added a comment -

          The css works fine in both FF and Chrome. I havent tried IE yet.

          At the moment I'm switching chat over to using a renderer instead of pulling blocks of html from a .php file. Once that is done I'll look at the possibility of making it a common renderer that can also be used by messaging and comments.

          Show
          Andrew Davis added a comment - The css works fine in both FF and Chrome. I havent tried IE yet. At the moment I'm switching chat over to using a renderer instead of pulling blocks of html from a .php file. Once that is done I'll look at the possibility of making it a common renderer that can also be used by messaging and comments.
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Andrew Davis added a comment -

          Ive added a few more commits that will ultimately get squished. I'm just pushing them to github to avoid having them only exist on my laptop.

          the chat module is now using a renderer. I've still got plenty of cleaning up and tweaking to do. There's unused code I haven't cleaned out yet plus I'm not quite happy with the look of the bubble chat. I do however feel like we're moving in the right direction. The overall flow of the code is much more straight forward.

          Show
          Andrew Davis added a comment - Ive added a few more commits that will ultimately get squished. I'm just pushing them to github to avoid having them only exist on my laptop. the chat module is now using a renderer. I've still got plenty of cleaning up and tweaking to do. There's unused code I haven't cleaned out yet plus I'm not quite happy with the look of the bubble chat. I do however feel like we're moving in the right direction. The overall flow of the code is much more straight forward.
          Hide
          Andrew Davis added a comment -

          Adding a new branch. Currently chat pulls in a config.php file from the theme directory that contain html fragments. I got rid of this system only to realize that I was making it really difficult to drop in additional chat themes. At present it is really easy to add new chat themes. I'm not sure if doing so is common however the capacity to do so is pretty neat.

          So, the new branch still switches chat over to using a renderer but it retains the use of the theme config.php files. It all works pretty nicely.

          I'm now synchronizing the html structures used by chat, messaging and comments. That's most likely where I will stop.

          Show
          Andrew Davis added a comment - Adding a new branch. Currently chat pulls in a config.php file from the theme directory that contain html fragments. I got rid of this system only to realize that I was making it really difficult to drop in additional chat themes. At present it is really easy to add new chat themes. I'm not sure if doing so is common however the capacity to do so is pretty neat. So, the new branch still switches chat over to using a renderer but it retains the use of the theme config.php files. It all works pretty nicely. I'm now synchronizing the html structures used by chat, messaging and comments. That's most likely where I will stop.
          Hide
          Andrew Davis added a comment -

          I think I've probably taken this about as far as is worthwhile. I've now made comments, messaging and chat use html structures that are at least very similar.

          Putting this up for peer review.

          Show
          Andrew Davis added a comment - I think I've probably taken this about as far as is worthwhile. I've now made comments, messaging and chat use html structures that are at least very similar. Putting this up for peer review.
          Hide
          Andrew Davis added a comment - - edited

          Gah. just noticed 2 small problems. There's a comment in mod/chat/renderer.php that refers to workshop. No php docs on the class definitions in mod/chat/locallib.php. I have them for the member variables but not the classes themselves. I'll fix these after lunch.

          update: fixed.

          Show
          Andrew Davis added a comment - - edited Gah. just noticed 2 small problems. There's a comment in mod/chat/renderer.php that refers to workshop. No php docs on the class definitions in mod/chat/locallib.php. I have them for the member variables but not the classes themselves. I'll fix these after lunch. update: fixed.
          Hide
          David Monllaó added a comment - - edited

          Hi Andrew,

          All looks great, only a pair of little output issues and a pair of doubts:

          • mod/chat/lib.php
            • The replacement rightalign value, known as tablealign in the gui_ajax/ themes config, assigns align="right" to a div, which works, but will generate invalid XHTML strict code
            • Not sure if the require_once('locallib.php') of chat_format_message_theme() could throw an error depending on from where chat/lib.php is required
          • There are inline styles in mod/chat/gui_ajax/theme/bubble/config.php
          • In mod/chat/locallib.php, event_message and user_message constructors don't have a description
          • mod/chat/renderer.php
            • No render_event_message method description
            • replacement pattern __mymessageclass__ doesn't exists in the core gui_ajax/ themes, I suppose is for 3rd party themes and it's still there for backward compatibility with 3rd party gui_ajax/ themes otherwise maybe it should be removed if the core themes are not using it?
          • About the speechbubbles CSS, I really don't know much about the way Moodle minifies CSS, but it seems that mod/chat/gui_ajax/theme/chat.css is not minified (I suppose because gui_chat/ themes are not subplugins with a styles.css) nor speechbubbles.css as well, I neither know the way the 3rd party libraries like speechbubbles gets into Moodle core, but minifying it and/or joining it with chat.css some HTTP requests could be avoided

          edited: tested in IE9 and IE9 with IE7 and IE8 browser mode, and the bubbles theme looks great

          Show
          David Monllaó added a comment - - edited Hi Andrew, All looks great, only a pair of little output issues and a pair of doubts: mod/chat/lib.php The replacement rightalign value, known as tablealign in the gui_ajax/ themes config, assigns align="right" to a div, which works, but will generate invalid XHTML strict code Not sure if the require_once('locallib.php') of chat_format_message_theme() could throw an error depending on from where chat/lib.php is required There are inline styles in mod/chat/gui_ajax/theme/bubble/config.php In mod/chat/locallib.php, event_message and user_message constructors don't have a description mod/chat/renderer.php No render_event_message method description replacement pattern __ mymessageclass __ doesn't exists in the core gui_ajax/ themes, I suppose is for 3rd party themes and it's still there for backward compatibility with 3rd party gui_ajax/ themes otherwise maybe it should be removed if the core themes are not using it? About the speechbubbles CSS, I really don't know much about the way Moodle minifies CSS, but it seems that mod/chat/gui_ajax/theme/chat.css is not minified (I suppose because gui_chat/ themes are not subplugins with a styles.css) nor speechbubbles.css as well, I neither know the way the 3rd party libraries like speechbubbles gets into Moodle core, but minifying it and/or joining it with chat.css some HTTP requests could be avoided edited: tested in IE9 and IE9 with IE7 and IE8 browser mode, and the bubbles theme looks great
          Hide
          Andrew Davis added a comment -

          I believe I have dealt with all that with the exception of css minifying. Thankyou for the thorough review

          I've introduced the __mymessageclass__ token into the bubble theme templates. We don't have any css that uses it but I could imagine someone wanting a token like that for their own themes.

          I'll contact Sam about CSS minification. I honestly have no idea how thats done within Moodle. This seems a good opportunity to find out.

          Show
          Andrew Davis added a comment - I believe I have dealt with all that with the exception of css minifying. Thankyou for the thorough review I've introduced the __ mymessageclass __ token into the bubble theme templates. We don't have any css that uses it but I could imagine someone wanting a token like that for their own themes. I'll contact Sam about CSS minification. I honestly have no idea how thats done within Moodle. This seems a good opportunity to find out.
          Hide
          Andrew Davis added a comment -

          Putting this up for peer review pending input from Sam about minification.

          Show
          Andrew Davis added a comment - Putting this up for peer review pending input from Sam about minification.
          Hide
          Andrew Davis added a comment -

          After speaking to Sam and doing some investigating I have moved the speech bubble CSS into the bubble theme's main css file. That means there's 1 css file instead of 2. I have also minified the third party css. I've put a comment above it saying that its minified and providing a link to the original.

          Show
          Andrew Davis added a comment - After speaking to Sam and doing some investigating I have moved the speech bubble CSS into the bubble theme's main css file. That means there's 1 css file instead of 2. I have also minified the third party css. I've put a comment above it saying that its minified and providing a link to the original.
          Hide
          David Monllaó added a comment -

          All looks good to me, I only see a missing "." before "text" in the .comment-message styles of theme/base/style/core.css, if I'm not wrong without the "." the style will not be applied

          Show
          David Monllaó added a comment - All looks good to me, I only see a missing "." before "text" in the .comment-message styles of theme/base/style/core.css, if I'm not wrong without the "." the style will not be applied
          Hide
          Andrew Davis added a comment -

          I've fixed that and added testing instructions. Submitting for integration.

          Show
          Andrew Davis added a comment - I've fixed that and added testing instructions. Submitting for integration.
          Hide
          Andrew Davis added a comment -

          Adding the ui_change label as the chat UI is somewhat different.

          Show
          Andrew Davis added a comment - Adding the ui_change label as the chat UI is somewhat different.
          Hide
          Aparup Banerjee added a comment -

          Thanks Andrew, i've integrated that into master now.

          I did some basic testing, and fixed some minor whitespace. its looking quiet cool for me

          ps: there seem to be some unfrakenly styles with the @packages doc here and there still.

          Show
          Aparup Banerjee added a comment - Thanks Andrew, i've integrated that into master now. I did some basic testing, and fixed some minor whitespace. its looking quiet cool for me ps: there seem to be some unfrakenly styles with the @packages doc here and there still.
          Hide
          Rajesh Taneja added a comment -

          Looks Good,

          Thanks for fixing this Andrew.

          Show
          Rajesh Taneja added a comment - Looks Good, Thanks for fixing this Andrew.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Fixed STOP Closed STOP Thanks STOP

          Yay, imagination! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: