Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Usability
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      • A book module with chapters and sub-chapters

      Test steps

      1. Explore the book as a teacher (editing mode on and off)
      2. Make sure:
        • The icons are nice and displayed
        • The fallback to PNG works
        • RTL is displayed correctly
        • The Table of Content is displayed correctly with any setting for _Chapter formatting _
      Show
      Test pre-requisites A book module with chapters and sub-chapters Test steps Explore the book as a teacher (editing mode on and off) Make sure : The icons are nice and displayed The fallback to PNG works RTL is displayed correctly The Table of Content is displayed correctly with any setting for _Chapter formatting _
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36628-master

      Description

      Book activity icon issues

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              fred Frédéric Massart added a comment -

              Add icon in the book block is red. Also padding is not good. See icon_book.png

              Show
              fred Frédéric Massart added a comment - Add icon in the book block is red. Also padding is not good. See icon_book.png
              Hide
              fred Frédéric Massart added a comment -

              What about:

              • Generate IMS
              • Print Book
              • Print Chapter
              • The navigation icons?
              Show
              fred Frédéric Massart added a comment - What about: Generate IMS Print Book Print Chapter The navigation icons?
              Hide
              barbararamiro Barbara Ramiro added a comment -

              Yes book navigation icons plus padding/margin between edit icons on the table of contents

              Show
              barbararamiro Barbara Ramiro added a comment - Yes book navigation icons plus padding/margin between edit icons on the table of contents
              Hide
              poltawski Dan Poltawski added a comment -

              Bah, was just about to duplicate this.

              Show
              poltawski Dan Poltawski added a comment - Bah, was just about to duplicate this.
              Hide
              fred Frédéric Massart added a comment -

              The Generate IMS icon has not been worked on yet, it's not as easy as using the Module icon in size 16, it has to be monochrome... Also, I cleaned up a lot the book CSS file which had a lot of repeated rules.

              Show
              fred Frédéric Massart added a comment - The Generate IMS icon has not been worked on yet, it's not as easy as using the Module icon in size 16, it has to be monochrome... Also, I cleaned up a lot the book CSS file which had a lot of repeated rules.
              Hide
              jfilip Justin Filip added a comment - - edited

              [Y] Syntax
              [Y] Output
              [Y] Whitespace
              [-] Language
              [-] Databases
              [Y] Testing
              [-] Security
              [-] Documentation
              [Y] Git
              [Y] Sanity check

              The code looks good to me. For the Documentation section, I'm not sure if this issue should have the ui_change label applied to it or not. It is a UI change but not one that would require documentation updates, unless someone wanted to take a screenshot with the new icons?

              And also a question: Fred, at one point in the CSS file you modified a line to change something from from margin-left: 0px; to margin-left: 0; and I was curious why that was changed:

              Show
              jfilip Justin Filip added a comment - - edited [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check The code looks good to me. For the Documentation section, I'm not sure if this issue should have the ui_change label applied to it or not. It is a UI change but not one that would require documentation updates, unless someone wanted to take a screenshot with the new icons? And also a question: Fred, at one point in the CSS file you modified a line to change something from from margin-left: 0px; to margin-left: 0; and I was curious why that was changed: https://github.com/FMCorz/moodle/compare/020e338...MDL-36628-master#L13L8
              Hide
              fred Frédéric Massart added a comment -

              Hi Justin, thanks for the review. Interesting point you raised about the ui_change label, I have discussed it and added the label on the meta issue, because most of the issues would need some documentation if we decide to. Also, the 0px to 0 was because my linter annoyed me saying that px is irrelevant when the value is 0. I usually didn't fix it, but here I did apparently.

              Pushing for integration, thanks!

              Show
              fred Frédéric Massart added a comment - Hi Justin, thanks for the review. Interesting point you raised about the ui_change label, I have discussed it and added the label on the meta issue, because most of the issues would need some documentation if we decide to. Also, the 0px to 0 was because my linter annoyed me saying that px is irrelevant when the value is 0. I usually didn't fix it, but here I did apparently. Pushing for integration, thanks!
              Hide
              poltawski Dan Poltawski added a comment -

              Fred: Meta is fine and probably most sensible in this case. But just to be clear, this label isn't just for us. Its so anyone who writes a book, or some training materials can see what changes in each release so they know where they need to update their screenshots etc.

              Show
              poltawski Dan Poltawski added a comment - Fred: Meta is fine and probably most sensible in this case. But just to be clear, this label isn't just for us. Its so anyone who writes a book, or some training materials can see what changes in each release so they know where they need to update their screenshots etc.
              Hide
              poltawski Dan Poltawski added a comment -

              Integrated, thanks fred.

              Show
              poltawski Dan Poltawski added a comment - Integrated, thanks fred.
              Hide
              markn Mark Nelson added a comment -

              Simply amazing. I am speechless. Passing.

              Show
              markn Mark Nelson added a comment - Simply amazing. I am speechless. Passing.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Y E S !

              Closing as fixed, many thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

                People

                • Votes:
                  0 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12