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

          Attachments

            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