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

      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().

        Gliffy Diagrams

          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: