Added frontend_type color #2945

fballiano · 2023-01-15T18:11:02Z

In #2937 it came the idea of having support for input type color for adminhtml forms, this PR adds it.

To test it, in a system.xml file, add some rows like these ones:

  <colortest>
      <frontend_type>color</frontend_type>
      <with_hash>1</with_hash><!-- 1 is default, saves the data with "#" as first character, if is set to 0 the "#" is not saved -->
      <label>Color</label>
      <show_in_default>1</show_in_default>
      <show_in_website>1</show_in_website>
      <show_in_store>1</show_in_store>
  </colortest>

then check the "system -> configuration" so see that it works, eg:

Related Pull Requests

Update jscolor to 2.5.1 #2937

justinbeaty · 2023-01-15T18:23:53Z

Oh nice one 👍. Tested and it works just fine.

Only thing to note. In @luigifab's PR, he has the option of color or color {hash:true}. The latter includes the # sign. However, the HTML5 element always uses the # sign. I don't see a way to enable or disable that with the HTML5 element.

Here's the results in the database:

+-----------+---------+----------+--------------------------+---------+------------+
| config_id | scope   | scope_id | path                     | value   | updated_at |
+-----------+---------+----------+--------------------------+---------+------------+
|       107 | default |        0 | system/csrf/colortest    | #a34343 | NULL       |
|       137 | default |        0 | system/csrf/color1       | EF5FFF  | NULL       |
|       138 | default |        0 | system/csrf/color2       | #B23EFF | NULL       |
+-----------+---------+----------+--------------------------+---------+------------+

colortest is this PR
color1 is jscolor without {hash:true}
color2 is jscolor with {hash:true}

But overall, I think this is a good addition, so I approve.

fballiano · 2023-01-15T18:25:41Z

I didn’t find anything in the specification about having the hash or not 😞

justinbeaty · 2023-01-15T18:36:43Z

I ran this SQL, to update the value from #ff0000 to ff0000

update core_config_data set value='ff0000' where path LIKE "%colortest";

Then when I reloaded the config page, the color picker was black instead of red.

So maybe, we can add this method to Color.php:

/**
 * @param string|null $index
 * @return string
 */
public function getEscapedValue($index = null)
{
    $value = parent::getEscapedValue($index);
    return '#' . ltrim($value, '#');
}

This would allow someone to use the color picker element on a value that doesn't have the hash in the database. Of course the new value would end up with the hash, so I don't know if that's going to mess up their code then. But, this is a new feature so it's not breaking anybody's code automatically.

Edit: Fixed the above code to call parent::getEscapedValue instead of parent::getValue

ADDISON74 · 2023-01-15T18:37:02Z

In HTML5 you just choose the color and the magic happens in the background. In jscolor you can both choose the color and enter it in a box. this makes the difference between the two. Adding a hash before the string makes it a hexadecimal value. More details here https://jscolor.com/docs/#api--opts--hash.

This PR is a great feature and I support it.

justinbeaty · 2023-01-15T18:38:33Z

In jscolor you can both choose the color and enter it in a box. this makes the difference between the two.

You can do that with HTML5 too (at least on Chrome), but it's a bit hidden:

ADDISON74 · 2023-01-15T18:42:59Z

In this case your proposal to check if a hexadecimal string is preceded by a hash appears to be logical. Maybe it should be updated in the database as well, but that would complicate things to be achieved.

ADDISON74 · 2023-01-15T18:46:20Z

In jscolor hash option is set to false by default

color : function(target, prop) {


		this.required = true; // refuse empty values?
		this.adjust = true; // adjust value to uniform notation?
		this.hash = false; // prefix color with # symbol?

fballiano · 2023-01-15T18:51:38Z

I ran this SQL, to update the value from #ff0000 to ff0000
update core_config_data set value='ff0000' where path LIKE "%colortest";
Then when I reloaded the config page, the color picker was black instead of red.

So maybe, we can add this method to Color.php:
/**
 * @param string|null $index
 * @return string
 */
public function getEscapedValue($index = null)
{
    $value = parent::getEscapedValue($index);
    return '#' . ltrim($value, '#');
}
This would allow someone to use the color picker element on a value that doesn't have the hash in the database. Of course the new value would end up with the hash, so I don't know if that's going to mess up their code then. But, this is a new feature so it's not breaking anybody's code automatically.

Edit: Fixed the above code to call parent::getEscapedValue instead of parent::getValue

great idea but has a problem, if my value is saved (initially) without the hash, I probably expect it to keep being saved without the hash...

I'll try to work something out

justinbeaty · 2023-01-15T18:56:22Z

great idea but has a problem, if my value is saved (initially) without the hash, I probably expect it to keep being saved without the hash...

I'll try to work something out

Yes, agreed it would be better to allow hash=true or hash=false. I suppose there's a way to do this with two inputs (one color and one text). I will see what you come up with.

fballiano · 2023-01-15T21:51:55Z

I've committed a version that manages the with_hash configuration, like this:

  <colortest>
      <frontend_type>color</frontend_type>
      <with_hash>1</with_hash><!-- 1 is default, saves the data with "#" as first character, if is set to 0 the "#" is not saved -->
      <label>Color</label>
      <show_in_default>1</show_in_default>
      <show_in_website>1</show_in_website>
      <show_in_store>1</show_in_store>
  </colortest>

it's done with an hidden input and a small bit of javascript. maybe not the super best way but at least I didn't have to add a backend model or anything more complicated.

what do you think?

justinbeaty · 2023-01-15T22:02:08Z

That's pretty neat! One problem I encountered. Set with_hash to 1, then select a color, save the config page, and save the page again without selecting a new color. Hash will be missing in the database.

justinbeaty · 2023-01-15T22:04:55Z

This fixes it:

        $html = '<input id="' . $id . '" type="text" name="' . $this->getName()
              . '" value="' . ($with_hash ? '#' : '') . $valueWithoutHash . '" ' . '/>' . "\n";

fballiano · 2023-01-15T22:08:43Z

@justinbeaty great!!!

ADDISON74 · 2023-01-15T23:23:25Z

I added the XML code in a system.xml file and got the color field in the Backend. Initially the color selector is set to RGB mode, but the user can switch to HEX. The hex value is predicated by #.

I went through several scenarios

with_hash=0 => the user deletes the hash and saves. In the database the value has the hash in front of it.
with_hash=1 => the user deletes the hash and saves. In the database the value has the hash in front of it.
delete the <with_hash> line from the systeml.xml file => the user deletes the hash and saves. In the database the value has the hash in front of it.

Seeing this behavior I deleted the # in front of the value and I did not save. When I used again the color picker, the value had # in front of it, which was not added by this PR. This came from browser. This behavior is found in all Chromium-based browsers (Chrome, Edge, ...). In the case of Firefox, the color selector is totally different, it does not have the HEX option.

Chromium-based

Firefox

justinbeaty · 2023-01-15T23:31:18Z

In the database the value has the hash in front of it.

Do you confirm this phpMyAdmin or mysql cli client? In Chrome, the hex value will always have the hash in the color picker.

justinbeaty · 2023-01-15T23:32:04Z

Also I have an idea for possible improvement, I'll test now.

justinbeaty · 2023-01-16T00:42:24Z

Based on feedback by Addison, and how the jscolor picker works, I tried some improvements. This lets you also manually type in the hex code in addition to using the color picker, as well as adds some validation and styles.

sreichel · 2023-01-16T00:48:49Z

it's done with an hidden input and a small bit of javascript. maybe not the super best way but at least I didn't have to add a backend model or anything more complicated.

what do you think?

I think we should add that models. 😛

Would Mage/Adminhtml/Model/System/Config/... be a better place?

Working on suggestion.

fballiano · 2023-01-19T10:08:38Z

would color_picker or color_selector be better? (not needed)

the picker is only a way to input a color, I think "color" is correct, it's also the name used by html5

as said in a comment ... the new class Varien_Data_Form_Element_Color should be in Mage_XYZ extending Varien_Data_Form_Element_Text

why tho?

fballiano · 2023-01-19T10:22:49Z

I've committed @justinbeaty's code for managing color also in products' attributes

justinbeaty · 2023-01-19T14:02:25Z

would color_picker or color_selector be better? (not needed)

the picker is only a way to input a color, I think "color" is correct, it's also the name used by html5

I agree color is correct, because we have date not date_selector, as well as file not file_picker, etc.

as said in a comment ... the new class Varien_Data_Form_Element_Color should be in Mage_XYZ extending Varien_Data_Form_Element_Text

why tho?

I also think lib/varien is good. At this point, lib/varien is Magento. I assume the company Varien never used this library for anything else and we would likely never split lib/varien from this repo. But if someone else ever did use lib/varien, then they could get a nice color picker. Plus, there are no references to Mage:: in the new class; if there were, then I'd agree it would be a Mage_Xyz class instead.

@justinbeaty after writing command for updating docblocks ... ❤️

... is it possible to extend your script to replace headers for all added files since last release to change the "author"?

this should be discussed in a proper discussion and/or rfc, I wouldn't block this PR which was supposed to be simple.

The script can be extended, but let's make a discussion about it.

I've committed @justinbeaty's code for managing color also in products' attributes

Thanks, although I was okay with making a separate PR. I should do a bit more testing to see how this attribute shows up in collections, etc. I don't foresee anything negative.

justinbeaty · 2023-01-19T18:57:37Z

@fballiano Let's revert Suggestions by justing, adding color also for products attributes 1fd0e08 and I will add it to a new PR after this is merged. There are improvements to be made there, plus I also see the commit is missing this file: app/code/core/Mage/Eav/Model/Entity/Attribute/Backend/Color.php.

…utes" This reverts commit 1fd0e08.

fballiano · 2023-01-19T19:00:30Z

done

fballiano · 2023-03-15T12:37:36Z

what are we doing with this PR? trash it?

justinbeaty · 2023-03-15T13:54:02Z

IIRC, it was ready to merge, just needed another approval.

elidrissidev · 2023-03-15T13:55:49Z

Will test right away

fballiano · 2023-03-15T16:57:23Z

yeah!

--------- Co-authored-by: Justin Beaty <51970393+justinbeaty@users.noreply.github.com> Co-authored-by: Sven Reichel <github-sr@hotmail.com>

fballiano · 2023-03-15T16:58:21Z

merged and cherrypicked to 20.0

Added frontend_type color

a6d9dfa

github-actions bot added Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* documentation labels Jan 15, 2023

fballiano added 2 commits January 15, 2023 18:11

fixed link to PR

48b8a8e

Removed maxlength

ebc0c69

justinbeaty previously approved these changes Jan 15, 2023

View reviewed changes

Added support for with_hash

d268b75

fballiano dismissed justinbeaty’s stale review via d268b75 January 15, 2023 21:46

Added support for with_hash

526bb4e

justinbeaty previously approved these changes Jan 15, 2023

View reviewed changes

sreichel mentioned this pull request Jan 16, 2023

Update jscolor to 2.5.1 #2937

Closed

4 tasks

fballiano added 2 commits January 19, 2023 10:18

Merge branch '1.9.4.x' into frontend_type_color

ad626dc

Suggestions by justing, adding color also for products attributes

1fd0e08

fballiano dismissed justinbeaty’s stale review via 1fd0e08 January 19, 2023 10:22

github-actions bot added Component: Catalog Relates to Mage_Catalog Component: Eav Relates to Mage_Eav labels Jan 19, 2023

Revert "Suggestions by justing, adding color also for products attrib…

116b4f8

…utes" This reverts commit 1fd0e08.

github-actions bot removed Component: Catalog Relates to Mage_Catalog Component: Eav Relates to Mage_Eav labels Jan 19, 2023

justinbeaty approved these changes Jan 19, 2023

View reviewed changes

sreichel approved these changes Jan 20, 2023

View reviewed changes

Merge branch '1.9.4.x' into frontend_type_color

accc6bc

fballiano requested review from elidrissidev and kiatng February 7, 2023 20:35

Merge branch '1.9.4.x' into frontend_type_color

5b34f1d

elidrissidev approved these changes Mar 15, 2023

View reviewed changes

fballiano merged commit 0e1c578 into OpenMage:1.9.4.x Mar 15, 2023

fballiano deleted the frontend_type_color branch March 15, 2023 16:57

fballiano added a commit that referenced this pull request Mar 15, 2023

Added frontend_type color (#2945)

bac869e

--------- Co-authored-by: Justin Beaty <51970393+justinbeaty@users.noreply.github.com> Co-authored-by: Sven Reichel <github-sr@hotmail.com>

fballiano mentioned this pull request Apr 7, 2023

Avoided calling getBackendModelByFieldConfig with empty field #3151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added frontend_type color #2945

Added frontend_type color #2945

fballiano commented Jan 15, 2023 •

edited

Loading

justinbeaty commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023 •

edited

Loading

ADDISON74 commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 15, 2023 •

edited

Loading

fballiano commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 16, 2023

sreichel commented Jan 16, 2023

fballiano commented Jan 19, 2023

fballiano commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

fballiano commented Jan 19, 2023

fballiano commented Mar 15, 2023

justinbeaty commented Mar 15, 2023

elidrissidev commented Mar 15, 2023

fballiano commented Mar 15, 2023

fballiano commented Mar 15, 2023

Added frontend_type color #2945

Added frontend_type color #2945

Conversation

fballiano commented Jan 15, 2023 • edited Loading

Related Pull Requests

justinbeaty commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023 • edited Loading

ADDISON74 commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

fballiano commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 15, 2023 • edited Loading

fballiano commented Jan 15, 2023

ADDISON74 commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 15, 2023

justinbeaty commented Jan 16, 2023

sreichel commented Jan 16, 2023

fballiano commented Jan 19, 2023

fballiano commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

justinbeaty commented Jan 19, 2023

fballiano commented Jan 19, 2023

fballiano commented Mar 15, 2023

justinbeaty commented Mar 15, 2023

elidrissidev commented Mar 15, 2023

fballiano commented Mar 15, 2023

fballiano commented Mar 15, 2023

fballiano commented Jan 15, 2023 •

edited

Loading

justinbeaty commented Jan 15, 2023 •

edited

Loading

justinbeaty commented Jan 15, 2023 •

edited

Loading