[#30743] - Fix color field attributes onchange and required.
- Closed
- 21 Jul 2022
- Medium
- Build: staging
- # 30743
- Diff
- Harmageddon:fix_color_field_attributes
- Success Download Prebuilt packages are available for download. Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success continuous-integration/drone/pr Build is passing Details
User tests: Successful: Unsuccessful:
Summary of Changes
- Fixed the attributes
onchangeandrequiredfor the color form field. - Improved the documentation in the layout files.
Testing Instructions
- In some kind of form, create or edit a form field of
type="color". Add the attributesrequiredand/oronchange. 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 - Open the form.
- Test the
requiredattribute: Try saving the form without entering a value in the required color field. - Test the
onchangeattribute: Select a value for the field that has theonchangeattribute and see whether the JavaScript handler is called. - 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
requiredattribute 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$controland$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?
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Layout |
| Labels |
Added:
?
|
||
@dgrammatiko Okay, thank you! I'll look into it tomorrow.
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.
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
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).
@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?
@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.
@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...
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.
@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.
Will close here for now. Thanks for the work done here :)
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-07-21 17:12:24 |
| Closed_By | ⇒ | zero-24 | |
| Labels |
Added:
?
Removed: ? |
||
You can never escape git blame? .
I really don't remember what these 2 attributes are doing but the script docs gives some hints:
Please check the docs, I might got it wrong https://labs.abeautifulsite.net/jquery-minicolors/#settings