Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Blog, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to Admin tree -> Appearance -> Blog
      2. Enable useblogassociations, useexternalblogs and blogusecomments
      3. Go to Navigation -> my profile -> blogs, create a blog entry adding a couple of tags and attaching an image file and a non-image file
      4. Go to Navigation -> my profile -> blogs and "View all of my entries", the added entry SHOULD appear, SHOULD contain links to the tags, SHOULD display the attached image and SHOULD show a link to the other attachment
      5. Go to Settings -> My profile settings -> Blogs -> "Register an external blog" and add a blog RSS feed
      6. Go to Navigation -> my profile -> blogs and "View all of my entries", it SHOULD appear your entry/ies with an "edit" link and the external blog entries without "edit" link
      7. Click the "Delete" link of an entry
      8. You SHOULD see the entry, you SHOULD NOT see any exceptions nor debugging messages and you SHOULD see a box below the entry to confirm the delete
      9. Go to a course, turn edition mode on and add the "Blog menu" block
      10. Click "Add an entry about this course" and fill the form
      11. After being redirected to view the entry you SHOULD be able to view a link to the associated course below the entry body and the tags
      12. Go to an activity, turn edition mode on and add the "Blog menu" block
      13. Click "Add an entry about this $MODULENAME" and fill the form
      14. After being redirected to view the entry you SHOULD be able to view a link to the associated activity below the entry body and the tags

      Please, test it with diferent themes and diferent browsers

      ps: a test with RTL in themes to see any effects from this blog change will be useful for creating any further issues .

      Show
      Go to Admin tree -> Appearance -> Blog Enable useblogassociations, useexternalblogs and blogusecomments Go to Navigation -> my profile -> blogs, create a blog entry adding a couple of tags and attaching an image file and a non-image file Go to Navigation -> my profile -> blogs and "View all of my entries", the added entry SHOULD appear, SHOULD contain links to the tags, SHOULD display the attached image and SHOULD show a link to the other attachment Go to Settings -> My profile settings -> Blogs -> "Register an external blog" and add a blog RSS feed Go to Navigation -> my profile -> blogs and "View all of my entries", it SHOULD appear your entry/ies with an "edit" link and the external blog entries without "edit" link Click the "Delete" link of an entry You SHOULD see the entry, you SHOULD NOT see any exceptions nor debugging messages and you SHOULD see a box below the entry to confirm the delete Go to a course, turn edition mode on and add the "Blog menu" block Click "Add an entry about this course" and fill the form After being redirected to view the entry you SHOULD be able to view a link to the associated course below the entry body and the tags Go to an activity, turn edition mode on and add the "Blog menu" block Click "Add an entry about this $MODULENAME" and fill the form After being redirected to view the entry you SHOULD be able to view a link to the associated activity below the entry body and the tags Please, test it with diferent themes and diferent browsers ps: a test with RTL in themes to see any effects from this blog change will be useful for creating any further issues .
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32344_master
    • Rank:
      39167

      Description

      Blog pages use tables to layout single blog texts. This is very bad from theming perspective. Could tables be replaced with divs?

      The rendering is done in /blog/locallib.php in function print_html().

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          Thanks for mentioning this. I will be creating a solution for 2.3 for this, rather than the previous versions, as it may break the current themes.

          Show
          Jason Fowler added a comment - Thanks for mentioning this. I will be creating a solution for 2.3 for this, rather than the previous versions, as it may break the current themes.
          Hide
          Jason Fowler added a comment -

          After discussing it with other developers at HQ, I will look in to converting the output into a renderer generated output, allowing theme developers to over ride the output.

          Show
          Jason Fowler added a comment - After discussing it with other developers at HQ, I will look in to converting the output into a renderer generated output, allowing theme developers to over ride the output.
          Hide
          Jason Fowler added a comment -

          Thanks for that David, it looks good, didn't test all of it, just the blog rendering in the default theme.

          Show
          Jason Fowler added a comment - Thanks for that David, it looks good, didn't test all of it, just the blog rendering in the default theme.
          Hide
          David Monllaó added a comment -

          Thanks for the review Jason, submitting for integration. I've only added the pull branch for master, if stable versions also needs the renderer please tell me

          Show
          David Monllaó added a comment - Thanks for the review Jason, submitting for integration. I've only added the pull branch for master, if stable versions also needs the renderer please tell me
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          David Monllaó added a comment -

          Branch rebased

          Show
          David Monllaó added a comment - Branch rebased
          Hide
          Aparup Banerjee added a comment - - edited

          Hi David,
          This is looking great. Yay bye bye tables .

          I wanted to mentioned some things though:

          • the left margin on a blog post : the td seems to be replaced with a div space. perhaps we can use CSS for better theme flexibility.
          • the attached files (images) will render on the far right with your patch atm. Originally the whole block would scale and the image would appear sometimes nearer to the edit links. i think its ok to fix the size, but what i'm seeing is that
            • perhaps we need to adjust some basic styles to our core themes (base, canvas)
          Show
          Aparup Banerjee added a comment - - edited Hi David, This is looking great. Yay bye bye tables . I wanted to mentioned some things though: the left margin on a blog post : the td seems to be replaced with a div space. perhaps we can use CSS for better theme flexibility. the attached files (images) will render on the far right with your patch atm. Originally the whole block would scale and the image would appear sometimes nearer to the edit links. i think its ok to fix the size, but what i'm seeing is that perhaps we need to adjust some basic styles to our core themes (base, canvas)
          Hide
          Aparup Banerjee added a comment -

          reopening for further look into this.

          Show
          Aparup Banerjee added a comment - reopening for further look into this.
          Hide
          CiBoT added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          David Monllaó added a comment -

          Thanks Aparup,

          • It follows the forum post style, so I removed the left floater div and added a new class to maintain the margin assigned to ".forumpost .row .left" class
          • Also following the forum style, I moved the attachments div at the upper part and I changed the class of the attached images to ".attachedimages". I've also have refined the used classes to ensure it works like the forum in all the core themes.

          Submitting for integration

          Show
          David Monllaó added a comment - Thanks Aparup, It follows the forum post style, so I removed the left floater div and added a new class to maintain the margin assigned to ".forumpost .row .left" class Also following the forum style, I moved the attachments div at the upper part and I changed the class of the attached images to ".attachedimages". I've also have refined the used classes to ensure it works like the forum in all the core themes. Submitting for integration
          Hide
          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
          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
          David Monllaó added a comment -

          Pull branches rebased

          Show
          David Monllaó added a comment - Pull branches rebased
          Hide
          Aparup Banerjee added a comment -

          Hi David,
          Thanks for showing themers some love .
          This is looking almost ready to go , there's just one more thing we've to cater to:
          theme/base/style/core.css :

           
          .blog_entry .content {margin-left: 43px !important;} 
          

          The use of !important here will prevent any theme extending base to use a right margin instead and have no left margin (perhaps when considering RTL languages).
          We have to be very careful when using this one addition weight point within core themes as this could easily lead to lots of '!important' being used by themes.

          If there isn't a real need for the "!important" here i'd take that out.

          ps: How does this change affect RTL btw? Perhaps we can have it in tests and create more issues if needed since this is for master only.

          Show
          Aparup Banerjee added a comment - Hi David, Thanks for showing themers some love . This is looking almost ready to go , there's just one more thing we've to cater to: theme/base/style/core.css : .blog_entry .content {margin-left: 43px !important;} The use of !important here will prevent any theme extending base to use a right margin instead and have no left margin (perhaps when considering RTL languages). We have to be very careful when using this one addition weight point within core themes as this could easily lead to lots of '!important' being used by themes. If there isn't a real need for the "!important" here i'd take that out. ps: How does this change affect RTL btw? Perhaps we can have it in tests and create more issues if needed since this is for master only.
          Hide
          David Monllaó added a comment -

          Thanks for the info Aparup, I've removed the !important from .blog_entry .content (it was there to overwrite an arialist theme style) and I've also added the style to the arialist theme below the one how has to overwrite. Tested in all the core themes.

          Show
          David Monllaó added a comment - Thanks for the info Aparup, I've removed the !important from .blog_entry .content (it was there to overwrite an arialist theme style) and I've also added the style to the arialist theme below the one how has to overwrite. Tested in all the core themes.
          Hide
          Aparup Banerjee added a comment - - edited

          excellent David , i've integrated that into master now.

          i've also added a note for RTL testing to test intructions so we can follow up this improvement with any further needed refinements there.

          note: ui_change was added here - but really this is a change affecting themes out there.

          Show
          Aparup Banerjee added a comment - - edited excellent David , i've integrated that into master now. i've also added a note for RTL testing to test intructions so we can follow up this improvement with any further needed refinements there. note: ui_change was added here - but really this is a change affecting themes out there.
          Hide
          David Monllaó added a comment -

          Tester: I've created http://tracker.moodle.org/browse/MDL-34869 to work on the RTL support

          Show
          David Monllaó added a comment - Tester: I've created http://tracker.moodle.org/browse/MDL-34869 to work on the RTL support
          Hide
          Mary Evans added a comment -

          Can I ask why .blog_entry .content

          {margin-left: 43px;}

          was added to arialist/style/core.css when you also added the same to base/style/core.css?

          Any changes should only go into base theme, so can you please remove the change to Arialist theme as it should not be needed since it will inherit it from Base theme, it's only duplicating CSS otherwise.

          Thanks

          Mary

          Show
          Mary Evans added a comment - Can I ask why .blog_entry .content {margin-left: 43px;} was added to arialist/style/core.css when you also added the same to base/style/core.css? Any changes should only go into base theme, so can you please remove the change to Arialist theme as it should not be needed since it will inherit it from Base theme, it's only duplicating CSS otherwise. Thanks Mary
          Hide
          Mary Evans added a comment -

          Further to my last comment:

          Reading all that has gone on here, would it not have made better sense to ask Theme designers how layouts should be built so that styling is kept to a minimum. There are some very bad layouts in Moodle that are a pain to style because of the way they were built when changing to a table-less design, forums being the worst, with all the 'left' and 'row' classes peppered about the place.

          Show
          Mary Evans added a comment - Further to my last comment: Reading all that has gone on here, would it not have made better sense to ask Theme designers how layouts should be built so that styling is kept to a minimum. There are some very bad layouts in Moodle that are a pain to style because of the way they were built when changing to a table-less design, forums being the worst, with all the 'left' and 'row' classes peppered about the place.
          Hide
          Aparup Banerjee added a comment -

          Hi Mary,
          Thanks for taking a look at this. I think you're right, we should incorporate theme designers opinion better when we're involved with styles and CSS changes. perhaps we could incorporate theme_change as a label which can be picked or better, have additional peer-reviewer fields added ('theme reviewer'?).

          The css David added to arialist was since there was a style (forum-post) in there overriding the base style he added. I think any styling difficultly should be fixed up for sure, maybe a separate issue could be made to deal with it generally as i'm not aware of the details of this problem.

          Show
          Aparup Banerjee added a comment - Hi Mary, Thanks for taking a look at this. I think you're right, we should incorporate theme designers opinion better when we're involved with styles and CSS changes. perhaps we could incorporate theme_change as a label which can be picked or better, have additional peer-reviewer fields added ('theme reviewer'?). The css David added to arialist was since there was a style (forum-post) in there overriding the base style he added. I think any styling difficultly should be fixed up for sure, maybe a separate issue could be made to deal with it generally as i'm not aware of the details of this problem.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in master only.

          I tested the blog list in the following themes:

          • Afterburner
          • Arialist
          • Brick
          • Formal white
          • Fusion
          • Magazine
          • MyMobile
          • Nimble
          • Nonzero
          • Overlay
          • Sky High
          • Splash

          The blog entry presentation looked consistent.

          I also tested this in an RTL langauge (ar) and it looked OK, but I am not RTL expert.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in master only. I tested the blog list in the following themes: Afterburner Arialist Brick Formal white Fusion Magazine MyMobile Nimble Nonzero Overlay Sky High Splash The blog entry presentation looked consistent. I also tested this in an RTL langauge (ar) and it looked OK, but I am not RTL expert.
          Hide
          Mary Evans added a comment - - edited

          @Aparup

          I like the idea of a 'theme reviewer' this makes a lot of sense, and would, I am sure, be very useful to all concerned with making Moodle look good as well as work correctly.

          Thanks

          Show
          Mary Evans added a comment - - edited @Aparup I like the idea of a 'theme reviewer' this makes a lot of sense, and would, I am sure, be very useful to all concerned with making Moodle look good as well as work correctly. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: