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

Lines between RSS feed items are not showing in all themes

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.4
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Themes
    • Labels:
    • Environment:
      CentOS, PHP 5.3.6, MySQL 5.1.57, Any Browser
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Note to Tester:
      Needs RSS Feeds setting up prior to testing.

      1. TEST that a line appears between feeds in all themes.
      Show
      Note to Tester: Needs RSS Feeds setting up prior to testing. TEST that a line appears between feeds in all themes.
    • Workaround:
      Hide

      Make sure every feed item is short so that they only take up one line each.

      Show
      Make sure every feed item is short so that they only take up one line each.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-28561_M26

      Description

      In the Standard Theme, lines show between the feed items in the RSS Feed Block. This is good.

      I want to use a different theme, in particular, Formal White. I have noticed that lines do not show between feed items, which makes this block's content difficult to read (see attached image.) I believe that lines between feed items should always show, unless they violate some important design aspect of the overall theme. I don't see this being the case for Formal White since lines are used to separate topics.

      Replication instructions:

      1. Add my feed (or any other) to the feed database. http://www.rjerz.com/c/opsmgt/Podcasts/OMU6K100.xml
      2. Add a RSS feed block, and pick the feed.
      3. Switch back and forth between Standard and Formal White. You should see the problem.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I suspect this is more of an issue with themes than with the RSS block itself, but perhaps Dan can check.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I suspect this is more of an issue with themes than with the RSS block itself, but perhaps Dan can check.
            Hide
            rjerz Rick Jerz added a comment -

            Michael,

            Yes, I think you are correct that this is a theme problem since the lines do show in the Standard theme. Can you move this to the Themes area?

            Show
            rjerz Rick Jerz added a comment - Michael, Yes, I think you are correct that this is a theme problem since the lines do show in the Standard theme. Can you move this to the Themes area?
            Hide
            poltawski Dan Poltawski added a comment -

            Reassigning to themes

            Show
            poltawski Dan Poltawski added a comment - Reassigning to themes
            Hide
            daniss Daniele Cordella added a comment -

            formal_white fixed tis issue in MDL-29367

            Show
            daniss Daniele Cordella added a comment - formal_white fixed tis issue in MDL-29367
            Hide
            rjerz Rick Jerz added a comment -

            I think that this has been fixed (or addressed) in Moodle 2.3.1+.

            Show
            rjerz Rick Jerz added a comment - I think that this has been fixed (or addressed) in Moodle 2.3.1+.
            Hide
            rjerz Rick Jerz added a comment -

            I do see that MDL-29367, is maybe a duplicate of this MDL, and that MDL-29367 is closed.

            Show
            rjerz Rick Jerz added a comment - I do see that MDL-29367 , is maybe a duplicate of this MDL, and that MDL-29367 is closed.
            Hide
            lazydaisy Mary Evans added a comment -

            @Rick

            I've just come across this hiding in moodle.com.
            I don't think this has been fixed in Base theme which is where it should have been fixed originally.

            Show
            lazydaisy Mary Evans added a comment - @Rick I've just come across this hiding in moodle.com. I don't think this has been fixed in Base theme which is where it should have been fixed originally.
            Hide
            rjerz Rick Jerz added a comment -

            Mary, I think that you are correct. The lines do exist in Formal White, but I checked, and they are not in Afterburner and Anomaly. When I started this Issue, I was only concerned about my favorite theme. I agree that there should be some consistency between themes, unless a theme has good reason to go in a different direction (for creativity reasons). Maybe ideally, this could be an overall setting, somewhere. So I am not sure if this Issue should be closed, and a new one started, or if it is easier to keep this one alive.

            So maybe some features and settings to consider would be:
            1) Show lines between feed items.
            2) Sort the feed (dropdown items are): unsorted, ascending by date, descending by date, ascending by topic, descending by topic. (probably more) By the way, this sorting feature is already a tracker ISSUE (MDL-28591).
            3) (not really a feature, but a problem) See MDL-28633.

            Show
            rjerz Rick Jerz added a comment - Mary, I think that you are correct. The lines do exist in Formal White, but I checked, and they are not in Afterburner and Anomaly. When I started this Issue, I was only concerned about my favorite theme. I agree that there should be some consistency between themes, unless a theme has good reason to go in a different direction (for creativity reasons). Maybe ideally, this could be an overall setting, somewhere. So I am not sure if this Issue should be closed, and a new one started, or if it is easier to keep this one alive. So maybe some features and settings to consider would be: 1) Show lines between feed items. 2) Sort the feed (dropdown items are): unsorted, ascending by date, descending by date, ascending by topic, descending by topic. (probably more) By the way, this sorting feature is already a tracker ISSUE ( MDL-28591 ). 3) (not really a feature, but a problem) See MDL-28633 .
            Hide
            lazydaisy Mary Evans added a comment -

            I need to get this fixed! I am so behind with lots of stuff here.

            Show
            lazydaisy Mary Evans added a comment - I need to get this fixed! I am so behind with lots of stuff here.
            Hide
            lazydaisy Mary Evans added a comment -

            Sorry I totally forgot about this.

            Show
            lazydaisy Mary Evans added a comment - Sorry I totally forgot about this.
            Hide
            lazydaisy Mary Evans added a comment -

            Just wondering why Daniel did add these to Base theme as it would have saved a whole lot of time.

            Lest I forget again!

            .block_rss_client .list li:first-child {
                border-top-width: 0;
            }
             
            .block_rss_client .list li {
                border-top: 1px solid #DDDDDD;
                padding: 5px;
            }

            Show
            lazydaisy Mary Evans added a comment - Just wondering why Daniel did add these to Base theme as it would have saved a whole lot of time. Lest I forget again! .block_rss_client .list li:first-child { border-top-width: 0; }   .block_rss_client .list li { border-top: 1px solid #DDDDDD; padding: 5px; }
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mary,

            This CSS is in the wrong place. We should use the style file for the block for this.

            See block/html/styles.css for example.

            Please send your issue for peer review before sedning to itnegration.

            Show
            poltawski Dan Poltawski added a comment - Hi Mary, This CSS is in the wrong place. We should use the style file for the block for this. See block/html/styles.css for example. Please send your issue for peer review before sedning to itnegration.
            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
            lazydaisy Mary Evans added a comment -

            If that is the case, then half of base theme css should be in their respective core styles.css stylesheets.

            Show
            lazydaisy Mary Evans added a comment - If that is the case, then half of base theme css should be in their respective core styles.css stylesheets.
            Hide
            lazydaisy Mary Evans added a comment -

            Can someone please Peer Review my patch for this fix?
            Cheers
            Mary

            Show
            lazydaisy Mary Evans added a comment - Can someone please Peer Review my patch for this fix? Cheers Mary
            Hide
            lazydaisy Mary Evans added a comment -

            Rick Jerz Sorry this is taking so long, but as you will see in the comments it easier watching paint dry!

            Show
            lazydaisy Mary Evans added a comment - Rick Jerz Sorry this is taking so long, but as you will see in the comments it easier watching paint dry!
            Hide
            rjerz Rick Jerz added a comment -

            Mary, no problems with the delay. I understand what is happening. Also, remember that this problem has been fixed already in Formal White (which I use). It will be nice when the improvement is made globally to all themes.

            Show
            rjerz Rick Jerz added a comment - Mary, no problems with the delay. I understand what is happening. Also, remember that this problem has been fixed already in Formal White (which I use). It will be nice when the improvement is made globally to all themes.
            Hide
            lazydaisy Mary Evans added a comment -

            The thing is Rich this should have been fixed in the rss block styles.css instead of Standard theme. And why was it not suggested to fix it correctly when Daniele fixed this in Formal White?

            I am really mad about this.

            Show
            lazydaisy Mary Evans added a comment - The thing is Rich this should have been fixed in the rss block styles.css instead of Standard theme. And why was it not suggested to fix it correctly when Daniele fixed this in Formal White? I am really mad about this.
            Hide
            lazydaisy Mary Evans added a comment -

            Just added Sam Hemelryk as a watcher.

            @Sam, can I ask what were the reasons for deleting blocks/rss_client/styles.css and adding the CSS to Standard theme?

            https://github.com/moodle/moodle/commit/90723839ca88d09fe0b75a499bdea88367e737fc

            I only ask as I have been told to add blocks/rss_client/styles.css so that I can add the css back rather than add it to base and bootstrapbase themes.

            Thanks
            mary

            Show
            lazydaisy Mary Evans added a comment - Just added Sam Hemelryk as a watcher. @Sam, can I ask what were the reasons for deleting blocks/rss_client/styles.css and adding the CSS to Standard theme? https://github.com/moodle/moodle/commit/90723839ca88d09fe0b75a499bdea88367e737fc I only ask as I have been told to add blocks/rss_client/styles.css so that I can add the css back rather than add it to base and bootstrapbase themes. Thanks mary
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Mary, absolutely feel free to move it back if required.
            I imagine I would have got rid of it originally because I deemed the styles in that file to be for visual styling and only necessary CSS was allowed in those styles.css files.
            If you've got CSS that would apply to all themes then feel free to add a styles.css containing it.

            Thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Mary, absolutely feel free to move it back if required. I imagine I would have got rid of it originally because I deemed the styles in that file to be for visual styling and only necessary CSS was allowed in those styles.css files. If you've got CSS that would apply to all themes then feel free to add a styles.css containing it. Thanks Sam
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Rossiani, is there any chance you could peer review this prior to Integration Review?
            Thanks
            Mary

            Show
            lazydaisy Mary Evans added a comment - Hi Rossiani, is there any chance you could peer review this prior to Integration Review? Thanks Mary
            Hide
            lazydaisy Mary Evans added a comment -

            All branches rebased on latest updates.

            Show
            lazydaisy Mary Evans added a comment - All branches rebased on latest updates.
            Hide
            lazydaisy Mary Evans added a comment -

            I have tested this myself on all themes and works OK. Peer Reviewers seem too few and too busy for such a trivial thing as this, so setting for Integration Review but not holding my breath.

            Show
            lazydaisy Mary Evans added a comment - I have tested this myself on all themes and works OK. Peer Reviewers seem too few and too busy for such a trivial thing as this, so setting for Integration Review but not holding my breath.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mary,

            Sorry to send it back again...

            Please remove the rules which are now duplicated from theme/standard/ and theme/formalwhite and are in the block style. That is why I wanted the rules moved to the rss block styles file, then we don't need to duplicate the same rules in 3 places and then our themes are just adjusting the colours or things that they need to override.

            (Sorry I missed the original location of that theme specific stylw)

            Show
            poltawski Dan Poltawski added a comment - Hi Mary, Sorry to send it back again... Please remove the rules which are now duplicated from theme/standard/ and theme/formalwhite and are in the block style. That is why I wanted the rules moved to the rss block styles file, then we don't need to duplicate the same rules in 3 places and then our themes are just adjusting the colours or things that they need to override. (Sorry I missed the original location of that theme specific stylw)
            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
            lazydaisy Mary Evans added a comment -

            Will do.

            Show
            lazydaisy Mary Evans added a comment - Will do.
            Hide
            lazydaisy Mary Evans added a comment -

            All done and dusted.

            Show
            lazydaisy Mary Evans added a comment - All done and dusted.
            Hide
            lazydaisy Mary Evans added a comment -

            Just updated and rebased all branches ready for next PULL.
            Thanks

            Show
            lazydaisy Mary Evans added a comment - Just updated and rebased all branches ready for next PULL. Thanks
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Just REBASED once more based on current updated master/MOODLE_25_STABLE/MOODLE_24_STABLE

            Show
            lazydaisy Mary Evans added a comment - - edited Just REBASED once more based on current updated master/MOODLE_25_STABLE/MOODLE_24_STABLE
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Mary, got there in the end!

            Integrated to master, 25 and 24

            Show
            poltawski Dan Poltawski added a comment - Thanks Mary, got there in the end! Integrated to master, 25 and 24
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mary,

            It looks to me like the lines between the items on clean theme are not coloured correctly. See this screenshot.

            Show
            poltawski Dan Poltawski added a comment - Hi Mary, It looks to me like the lines between the items on clean theme are not coloured correctly. See this screenshot.
            Hide
            lazydaisy Mary Evans added a comment -

            Can you create a new issue to fix Clean theme and I'll fix that in bootstrapbase/less later today. I have a dentist appointment this morning.

            Cheers

            Show
            lazydaisy Mary Evans added a comment - Can you create a new issue to fix Clean theme and I'll fix that in bootstrapbase/less later today. I have a dentist appointment this morning. Cheers
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Mary,

            I'm not happy about making a new issue for this as I think it makes the RSS items look ugly and quite striking. So i'm hoping for a patch for this one else we'll need to revert it.

            I'll ask in the dev chat if anyone is able to create a fix for it in your absence.

            Show
            poltawski Dan Poltawski added a comment - Hi Mary, I'm not happy about making a new issue for this as I think it makes the RSS items look ugly and quite striking. So i'm hoping for a patch for this one else we'll need to revert it. I'll ask in the dev chat if anyone is able to create a fix for it in your absence.
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited
            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Hello Dan, Here is the patch to fix line color. https://github.com/rajeshtaneja/moodle/commit/196e75a7d12df03b94e4c98f62b6fe53e084d3cf
            Show
            poltawski Dan Poltawski added a comment - Thanks Raj - I modified your patch sligthly after talking with Fred about getting the variable from less for the colour: http://git.moodle.org/gw?p=integration.git;a=blobdiff;f=theme/bootstrapbase/less/moodle/blocks.less;h=786f8243d71b9c1a65a7226891badb9e9af46ce7;hp=156efa75d25f772f9215e8f562832aba20266804;hb=33f9a60133a54e3da4831b664686387c1e1f9eab;hpb=fa8b1fa4a4b592056938110735205bd867f19f23 Back to testing.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Dan,

            It make sense, I should have used variables. Thanks for reminding me to use variables in other places.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Dan, It make sense, I should have used variables. Thanks for reminding me to use variables in other places.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Mary and Dan.

            Looks good to me. Passing...

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Mary and Dan. Looks good to me. Passing...
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for picking that up Raj

            Show
            poltawski Dan Poltawski added a comment - Thanks for picking that up Raj
            Hide
            damyon Damyon Wiese added a comment -

            This issue along with 77 of it's friends has been sent upstream and released to the world.

            Thankyou for your contribution.

            Show
            damyon Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13