? Success

User tests: Successful: Unsuccessful:

avatar Harmageddon
Harmageddon
23 Sep 2020

Summary of Changes

  • Fixed the attributes onchange and required for the color form field.
  • Improved the documentation in the layout files.

Testing Instructions

  1. In some kind of form, create or edit a form field of type="color". Add the attributes required and/or onchange. Test with both color field layouts (control="simple" for the simple version).
    Alternatively, you can use the demo plugin at https://github.com/Harmageddon/plg_demo_colorfield
  2. Open the form.
  3. Test the required attribute: Try saving the form without entering a value in the required color field.
  4. Test the onchange attribute: Select a value for the field that has the onchange attribute and see whether the JavaScript handler is called.
  5. Check the HTML source of the form for validity.

Actual result BEFORE applying this Pull Request

The values for the two attributes are printed directly in the source code, leading to invalid HTML like:

<input type="text" name="jform[params][color2]" id="jform_params_color2" value="none" placeholder="#rrggbb" class="minicolors hex" data-position="default" data-control="hue"alert('New value: ' + this.value); data-format="hex"/>

The onchange handler is not executed.

Required fields don't get the required attribute, neither the aria-required="true" attribute.

Expected result AFTER applying this Pull Request

Onchange handlers are executed correctly. Required fields get the required attribute.

Documentation Changes Required

The documentation of the color form field is very rudimentary and doesn't mention most of the available attributes. It should be improved overall.

Open Issues

  • The required attribute still has no effect for the simple color picker variant. The reason is that the empty value is not an empty string, but 'none', which counts as a value both for server-side and client-side validation. I'm not sure how to fix this.
  • I also fixed the documentation in the PHP code of the layouts files. For $control and $position, the original description was copy-pasted, so it should be changed to something more meaningful. However, I'm not sure about a good description here. Maybe @dgrammatiko can help here, as you created these layouts?
avatar Harmageddon Harmageddon - open - 23 Sep 2020
avatar Harmageddon Harmageddon - change - 23 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Sep 2020
Category Layout
avatar dgrammatiko
dgrammatiko - comment - 23 Sep 2020

Maybe @dgrammatiko can help here, as you created these layouts

You can never escape git blame ?.

I really don't remember what these 2 attributes are doing but the script docs gives some hints:

  • control: has a 'hue', so it should be the color selection part (the vertical one)
  • position: has a 'bottom left' so I guess it controls where the pop up will be positioned (relative to the input)

Please check the docs, I might got it wrong https://labs.abeautifulsite.net/jquery-minicolors/#settings

avatar Harmageddon Harmageddon - change - 23 Sep 2020
Labels Added: ?
avatar Harmageddon
Harmageddon - comment - 23 Sep 2020

@dgrammatiko Okay, thank you! I'll look into it tomorrow.

avatar Harmageddon Harmageddon - change - 24 Sep 2020
The description was changed
avatar Harmageddon Harmageddon - edited - 24 Sep 2020
avatar Harmageddon
Harmageddon - comment - 24 Sep 2020

Updated the docblock accordingly. The other open issue (required validation for the simple color picker) remains, but I'd regard it as separate from this PR. If this is regarded as a bug, it should be fixed in another PR.

avatar dgrammatiko
dgrammatiko - comment - 2 Sep 2021

@thednp hey, quick question: did you manage to get that wasm scss compiler working?

avatar thednp
thednp - comment - 2 Sep 2021

@thednp hey, quick question: did you manage to get that wasm scss compiler working?

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

avatar dgrammatiko
dgrammatiko - comment - 2 Sep 2021

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

The reason I asked you was a recent discussion at #35403
Glad to hear that you've figured it out ?

avatar dgrammatiko
dgrammatiko - comment - 2 Sep 2021

Some cleanup needed. I hope devs push this to J4.x as well.

Once you're ready let me know and I'll give it a test

avatar thednp
thednp - comment - 2 Sep 2021

Yes man, thank you for asking. It was a lot of PHP "autoresolve" work.

The reason I asked you was a recent discussion at #35403
Glad to hear that you've figured it out ?

I'm using your sass.js implementation with a custom script to run via a blank custom field and works excellent, the only caveat is file names: if multiple files in multiple locations have same name, it might skip some of them in the auto-resolve process, other than that, it's alright.

So yea, I'm not going for the scssphp implementation any time soon, as you nicely advised (thanks).

avatar Quy
Quy - comment - 2 Sep 2021

Some cleanup needed. I hope devs push this to J4.x as well.

@thednp Is this PR okay other than your suggested changes? Changes in v3.10 will be upmerged into v4.

avatar thednp
thednp - comment - 2 Sep 2021

@Quy if my concerns get resolved, all should be fine, I even tested the changes in regards to "onchange", successfully.

avatar Quy
Quy - comment - 2 Sep 2021

@thednp The author appears to be inactive on GitHub so if no response in a few days, then I will make the changes. Thank you so much for your feedback!!

avatar thednp
thednp - comment - 8 Sep 2021

@dgrammatiko do you think dart.sass would work instead of sass.js by medialize?

In my initial impression, I might need some webpack action to create a bundle similar to the one by medialize. Your thoughts?

avatar dgrammatiko
dgrammatiko - comment - 8 Sep 2021

@thednp it should, but I haven't tried it. And yes you will need some sort of bundler (I'm in favor of Rollup as their docs are way more approachable and also it's a better bundler for most use cases)

avatar thednp
thednp - comment - 9 Sep 2021

@thednp it should, but I haven't tried it. And yes you will need some sort of bundler (I'm in favor of Rollup as their docs are way more approachable and also it's a better bundler for most use cases)

I've done some tests into it and haven't managed to get to "consume" the node modules and expose them for web, I used the ESM (nppm install esm) which aims to bridge the node and ES APIs, so I'm open to any suggestion you might have.

avatar dgrammatiko
dgrammatiko - comment - 9 Sep 2021

@thednp I think dart sass is not yet ready for the browser: sass/dart-sass#25 (comment) so for the time stay with sass.js: medialize/sass.js#135

Also, there are some other side projects from the (dart) team so it might be a browser-ready version there, check their repo's and their branches...

avatar thednp
thednp - comment - 9 Sep 2021

I already checked that (sass/dart-sass#25 (comment)), however, even if I change files manually and replace all require() instances with their module code I'm still getting tons of errors.

I actually don't know how to bundle npm modules together the way I manage to do with ES6+ modules.

avatar zero-24
zero-24 - comment - 12 Jun 2022

@dgrammatiko @thednp can you please confirm what the status here is and whether it should be better closed here and moved to j4 as mentiond inline there are some changes between j3 and j4 already.

avatar zero-24
zero-24 - comment - 21 Jul 2022

Will close here for now. Thanks for the work done here :)

avatar zero-24 zero-24 - change - 21 Jul 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-07-21 17:12:24
Closed_By zero-24
Labels Added: ?
Removed: ?
avatar zero-24 zero-24 - close - 21 Jul 2022

Add a Comment

Login with GitHub to post a comment