Moodle
  1. Moodle
  2. MDL-23504

Transparency and RGB support to colour picker

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.4
    • Fix Version/s: 2.5
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as admin
      2. Navigate to theme with color picker (Home ► Site administration ► Appearance ► Themes ► Formal white)
      3. Try following values color values in color picker (Header background colour) and make sure they get saved.
        • whitesmoke
        • #0404E6
        • rgb(0,255,244)
        • rgba(0,255,244,0.5)
        • transparent
        • currentColor
        • inherit
        • hsl(207,38%,47%)
        • hsla(207,38%,47%,0.8)
      4. Try with some wrong color values and it should show error
        • whitesmokewrongcolor
        • #0404E61
        • #0404
        • rgb(0,255,244,11)
        • rgba(0,255,244,10)
      Show
      Login as admin Navigate to theme with color picker (Home ► Site administration ► Appearance ► Themes ► Formal white) Try following values color values in color picker (Header background colour) and make sure they get saved. whitesmoke #0404E6 rgb(0,255,244) rgba(0,255,244,0.5) transparent currentColor inherit hsl(207,38%,47%) hsla(207,38%,47%,0.8) Try with some wrong color values and it should show error whitesmokewrongcolor #0404E61 #0404 rgb(0,255,244,11) rgba(0,255,244,10)
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-23504

      Description

      Colour picker should support also transparent background-color property - theme designers often want to use background-color:transparent instead of some color.

      If you add to lib/adminlib.php class admin_setting_configcolourpicker extends admin_setting

      {...}

      / protected function validate($data) two lines

      } else if ($data=='transparent') {
      return $data;

      so that the validitation could be done with

      protected function validate($data) {
      if (preg_match('/^#?([a-fA-F0-9]

      {3}

      )

      {1,2}

      $/', $data)) {
      if (strpos($data, '#')!==0)

      { $data = '#'.$data; }

      return $data;
      } else if (preg_match('/^[a-zA-Z]

      {3, 25}

      $/', $data))

      { return $data; } else if ($data=='transparent') { return $data; }

      else if (empty($data))

      { return $this->defaultsetting; }

      else

      { return false; }

      }

      This way theme administrators can write value

      transparent

      to colour picker input box and it is accepted as valid background-color.

      Transparency should of course be used only for background colors (not for text colors) so it could be good to note in docs or some help file about this thing...

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Daniel Wahl added a comment -

            I believe that this ticket title should be updated to say "valid color support in colour picker". It's not possible to set rgb() or rgba() values with the function - although, oddly, they do work when selecting "preview".

            Show
            Daniel Wahl added a comment - I believe that this ticket title should be updated to say "valid color support in colour picker". It's not possible to set rgb() or rgba() values with the function - although, oddly, they do work when selecting "preview".
            Hide
            Daniel Wahl added a comment -

            I created a patch for this - but I don't see how to generate a pull request here. Here's the diff link:

            https://github.com/thedannywahl/moodle/compare/MDL-23504

            the patch allows "transparent", adds support for rgb() and rgba(), uses xdigit to check hex values instead of a-fA-F0-9 and compares a list of all valid HTML color names instead of regex. I see this is marked as 2.0 - but I've only tested it on master (currently 2.5dev)

            Show
            Daniel Wahl added a comment - I created a patch for this - but I don't see how to generate a pull request here. Here's the diff link: https://github.com/thedannywahl/moodle/compare/MDL-23504 the patch allows "transparent", adds support for rgb() and rgba(), uses xdigit to check hex values instead of a-fA-F0-9 and compares a list of all valid HTML color names instead of regex. I see this is marked as 2.0 - but I've only tested it on master (currently 2.5dev)
            Hide
            Mary Evans added a comment -

            @Rajesh

            This has been hanging around for a while now, and Danny Wahl has kindly just added a PATCH for this. Is there any chance you could take a look at this or pass it on to someone to Peer Review?

            Thanks

            Show
            Mary Evans added a comment - @Rajesh This has been hanging around for a while now, and Danny Wahl has kindly just added a PATCH for this. Is there any chance you could take a look at this or pass it on to someone to Peer Review? Thanks
            Hide
            Rajesh Taneja added a comment -

            Thanks Mary looking at this now.
            Is it fine if I assign this to you, as I can't be assignee and reviewer.

            Show
            Rajesh Taneja added a comment - Thanks Mary looking at this now. Is it fine if I assign this to you, as I can't be assignee and reviewer.
            Hide
            Rajesh Taneja added a comment -

            Thanks for the patch Danny,

            There are few things you might want to consider before this gets integrated. Let us know if you want me to do these changes.

            1. Code alignment needs to be fixed (Tab to be replaced with 4 spaces). https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7946
            2. Moodle don't follow camelcase convention, so color names can be all lowercase.
            3. Why do we need to create $colornames string from an array? Can't we just use in_array(strtolower($data), $colornames).
            4. Support for % values in rgb can also be added. https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7988
            5. rgba is supported in ie9+, and moodle is still supporting ie8, not sure if this can be backported.
            6. At this point you might want to consider adding support for HSL and HSLA (Might be of use to some theme developer's)

            Hope this helps.

            Show
            Rajesh Taneja added a comment - Thanks for the patch Danny, There are few things you might want to consider before this gets integrated. Let us know if you want me to do these changes. Code alignment needs to be fixed (Tab to be replaced with 4 spaces). https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7946 Moodle don't follow camelcase convention, so color names can be all lowercase. Why do we need to create $colornames string from an array? Can't we just use in_array(strtolower($data), $colornames). Support for % values in rgb can also be added. https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7988 rgba is supported in ie9+, and moodle is still supporting ie8, not sure if this can be backported. At this point you might want to consider adding support for HSL and HSLA (Might be of use to some theme developer's) Hope this helps.
            Hide
            Mary Evans added a comment -

            Hi Rajesh

            I've just assigned to me, so I can manage this.

            @Danny,
            If you are stuck with anything let me know.

            Thanks again
            Mary

            Show
            Mary Evans added a comment - Hi Rajesh I've just assigned to me, so I can manage this. @Danny, If you are stuck with anything let me know. Thanks again Mary
            Hide
            Daniel Wahl added a comment -

            @rajesh:

            1) fixed
            2) fixed
            3) done, thanks!
            4) done
            5) doesn't seem like an issue to me - it's up to site administrators to enter rgba() or not.
            6) done

            patch updated

            Show
            Daniel Wahl added a comment - @rajesh: 1) fixed 2) fixed 3) done, thanks! 4) done 5) doesn't seem like an issue to me - it's up to site administrators to enter rgba() or not. 6) done patch updated
            Hide
            Daniel Wahl added a comment -

            Forgot to say I also added support for "currentColor" (CSS3) and "inherit"

            Show
            Daniel Wahl added a comment - Forgot to say I also added support for "currentColor" (CSS3) and "inherit"
            Hide
            Rajesh Taneja added a comment -

            Thanks Daniel,
            That was quick

            Show
            Rajesh Taneja added a comment - Thanks Daniel, That was quick
            Hide
            Rajesh Taneja added a comment -

            Sorry Daniel,

            There are few syntax problems:

            1. https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7990 should be else if (with space)
            2. Same with https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7992
            3. IMO https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7994 is not valid condition. If should be

              else if (($data === 'transparent') ||
                  ($data === 'currentColor') ||
                  ($data === 'inherit')) {
              

            Also, it might be nice to merge commits.
            Thanks for fixing this

            Show
            Rajesh Taneja added a comment - Sorry Daniel, There are few syntax problems: https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7990 should be else if (with space) Same with https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7992 IMO https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7994 is not valid condition. If should be else if (($data === 'transparent') || ($data === 'currentColor') || ($data === 'inherit')) { Also, it might be nice to merge commits. Thanks for fixing this
            Hide
            Daniel Wahl added a comment -

            fixed all those things.

            Show
            Daniel Wahl added a comment - fixed all those things.
            Hide
            Rajesh Taneja added a comment -

            Thanks Daniel,

            Sorry to bother you again. https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7992 it should be hsla.

            Also, IMHO if user put space between values in rgb/rgba, it should be valid.

            Show
            Rajesh Taneja added a comment - Thanks Daniel, Sorry to bother you again. https://github.com/thedannywahl/moodle/compare/master...MDL-23504#L0R7992 it should be hsla. Also, IMHO if user put space between values in rgb/rgba, it should be valid.
            Hide
            Daniel Wahl added a comment -

            fixed both of those issues.

            Show
            Daniel Wahl added a comment - fixed both of those issues.
            Hide
            Rajesh Taneja added a comment -

            Thanks for quick fix Daniel,

            Pushing it for integration review.

            Show
            Rajesh Taneja added a comment - Thanks for quick fix Daniel, Pushing it for integration review.
            Hide
            Sam Hemelryk added a comment -

            Hi Daniel,

            Thanks for the great effort here.

            I've just been looking at this now, we are presently in a sync period in which for 3 weeks after a release we keep our master branch as close to our MOODLE_24_STABLE branch as possible.
            Because of that master only changes get delayed until the end of that period (its 1 week since release so 2 to go).
            I've added the integration_held label so that we know this is a master only change and will need to wait.

            Looking over the changes the only thing I noted was the commit messages, we require that all commit messages conform to our guidelines here: http://docs.moodle.org/dev/Commit_cheat_sheet
            In short they must contain the tracker issue number and have a meaningful commit message.
            Could you please fix up the commit messages?
            Alternatively if you like we can squash it into a single commit and give it a suitable message during integration, please let us know if you would like us to do that.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Hi Daniel, Thanks for the great effort here. I've just been looking at this now, we are presently in a sync period in which for 3 weeks after a release we keep our master branch as close to our MOODLE_24_STABLE branch as possible. Because of that master only changes get delayed until the end of that period (its 1 week since release so 2 to go). I've added the integration_held label so that we know this is a master only change and will need to wait. Looking over the changes the only thing I noted was the commit messages, we require that all commit messages conform to our guidelines here: http://docs.moodle.org/dev/Commit_cheat_sheet In short they must contain the tracker issue number and have a meaningful commit message. Could you please fix up the commit messages? Alternatively if you like we can squash it into a single commit and give it a suitable message during integration, please let us know if you would like us to do that. Many thanks Sam
            Hide
            Daniel Wahl added a comment -

            If you wouldn't mind squashing them for me that would be great - I can't think of a reason to keep them as separate commits any more.

            And the cheat sheet has been added to the bookmarks! It's as difficult as anything to figure out "how-to" commit to Moodle via the docs...

            Show
            Daniel Wahl added a comment - If you wouldn't mind squashing them for me that would be great - I can't think of a reason to keep them as separate commits any more. And the cheat sheet has been added to the bookmarks! It's as difficult as anything to figure out "how-to" commit to Moodle via the docs...
            Hide
            Rajesh Taneja added a comment -

            Hi Sam and Daniel,

            I have created branch from Daniel's commits.

            Show
            Rajesh Taneja added a comment - Hi Sam and Daniel, I have created branch from Daniel's commits.
            Hide
            Sam Hemelryk added a comment -

            Thanks guys, this has been integrated now

            Show
            Sam Hemelryk added a comment - Thanks guys, this has been integrated now
            Hide
            Mark Nelson added a comment -

            Works as expected. Passing. One point however, if you enter a wrong value and click to submit you are given a warning that the value is invalid, but are shown the value that was previously saved. Standard behaviour is to show the field they have entered that was wrong with the warning message.

            Show
            Mark Nelson added a comment - Works as expected. Passing. One point however, if you enter a wrong value and click to submit you are given a warning that the value is invalid, but are shown the value that was previously saved. Standard behaviour is to show the field they have entered that was wrong with the warning message.
            Hide
            Daniel Wahl added a comment -

            Mark,

            That sounds like another issue to be created, all this patch does is extend the acceptable values, but doesn't address error handling.

            Show
            Daniel Wahl added a comment - Mark, That sounds like another issue to be created, all this patch does is extend the acceptable values, but doesn't address error handling.
            Hide
            Mark Nelson added a comment -

            Hi Daniel, thanks for your contribution. I have created a separate tracker issue which I have linked to this issue. Regards.

            Show
            Mark Nelson added a comment - Hi Daniel, thanks for your contribution. I have created a separate tracker issue which I have linked to this issue. Regards.
            Hide
            Dan Poltawski added a comment -

            Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            Show
            Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

              People

              • Votes:
                6 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: