-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.2] Adding Color Picker re-re-based #35639
Conversation
@dgrammatiko this is a completely rebased commit, still DART-SASS issues. As you can see, all changes are all about removing minicolors and replacing it with our color-picker. There is nothing wrong here. |
That's weird. The SASS package is the one that is defined in the package.json on the root of the repo and it doesn't throw any errors for the same files. Maybe an app like https://www.sourcetreeapp.com can help you here (figure out the point that your branch is created, normally if this is a new PR that should be the tip of the 4.1-dev.) |
Probably changing the SCSS sources too much will result in the issue, but I found another problem. In my local repository, I didn't do the |
The composer install needs to be done before the npm install. That's expected |
Something isn't right. EDIT: I just did a EDIT 2: it must be related to what @brianteeman said yesterday, there were some files changed unrelated to my commit. |
Guys, I can nail down these pesky SCSS issues real quick ( Just let me know. |
@thednp please make another PR that will change the division according to SASS docs: https://sass-lang.com/documentation/breaking-changes/slash-div So your example $font-size: $some-var / 2; should be written as: $font-size: math.div($some-var, 2); |
That won't work with our |
@dgrammatiko 5min into the job: Do we fix/resolve 3rd party code quality? |
Not in this repo. We can either do a PR to their repo or wait till someone else fixes it... |
We can just override the value with For instance joomla-cms\build\media_source\com_media\scss\components_media-infobar.scss overrides |
If you can override it then should be fine, we already overriding the path, etc |
We may have to override the entire |
You can not remove the minicolors css or js as they may be used by extensions and they have not been marked as deprecated |
So instead of removing minicolors assets and references in the asset json files, we have our color-picker separate? |
No that's not what Brian meant. The js+css needs to be in the media/vendor folder but not in the assets.josn. Basically you need to revert some of the changes in the build/settings.json... |
build/media_source/system/js/fields/color-field-adv-init.es5.js
Outdated
Show resolved
Hide resolved
Thanks @PhilETaylor |
Reverting deletion of minicolors in templates.
@dgrammatiko man, I put back minicolors and still no go. I think the problem is very much related to the build/media_source/system/scss/fields/joomla-field-color-picker.scss. It may be a problem with the SCSS + JS compile function in the |
@thednp has nothing to do with your code, the failure is in bootstrap.scss FWIW if you apply both the PRs from #35639 (comment) CI will be fine |
Alright buddy, what about my fixes in #35648? Also, any news on the TinyMCE improvements? Anything I can give it a try or input? |
That is valid (I think I'm not a SCSS person)
There will be a POC PR today, I'm just trying to introduce all the changes without an B/C breaks or a bazzilion of cryptic options |
To my research, those fixes are absolutely needed in dart-sass 2.0, it would be a waste of somebody's time someday to find and nail them down like I did.
Sweet, please tag me in. |
This pull request has automatically rebased to 4.2-dev. |
This pull request has been automatically rebased to 5.0-dev. |
This pull request has been automatically rebased to 5.1-dev. |
This pull request has been automatically rebased to 5.2-dev. |
Due to some complications, the previous commit didn't work.
Pull Request for Issue #32494.
Demo page of the color-picker component right here.
Summary of Changes
minicolors
assetstinycolor
assetjoomla-field-color-picker.es6.js
as described in [NEW] The future color picker for Joomla is here #32494layouts/joomla/form/field/color/advanced.php
fileTesting Instructions
JavaScript Testing
The most important test for me, please report any error in the console along with the following info:
Existing Extensions Testing
<field type="Color"
control="advanced">
field;<color-picker>
updates the background of the visible input with the value stored in the database; if not the script is missing, not initialized or something happened, see below;ColorPicker
orTinyColor
related errors; Report back here any error you encounter; I really hope there won't be;.color-dropdown
then click anywhere outside of it, the.color-dropdown
should close;.color-dropdown
element is visible try and resize the window, see if the console pops any error;.color-dropdown
element is rendered (above or below the visible input), then scroll the window up/down while open to check if the.color-dropdown
element should be re-positioned where it has the most available space;Color Picker Options Testing
/templates/cassiopeia/templateDetails.xml
, find the<fieldset name="advanced">
tag and paste inside the following code:<field name="advanced-color-hex">
sets thekeywords="false"
which disables the usage of presets and restricts the color-picker to only useformat="hex"
colors (in this particular case); the reason for this behavior is the need to send SCSS compilers real and valid HEX/RGB(a)/HSL(a) color values to be used for mixins, eg.darken($primary,15%)
(darken('inherit',15%)
won't work); this field should use a placeholder;<field name="advanced-color-rgb">
does not set thekeywords
option and will use the default keywords:transparent
,curentColor
,initial
andinherit
; as a result, the color-picker will display a button on the right side of the visible input to open a color options menu; these "non-color" values are valid CSS values; this field is required;<field name="advanced-color-hsl">
sets thekeywords="navy,crimson.."
and enables the usage of user defined presets based on web safe color names; because none of the above "non-color" keywords is present in the setting value, color-picker will again restrict values to only valid HSL(a) format colors for the same known reasons.Keyboard Events Testing
.color-dropdown
is displayed, hit the ESC key of your keyboard, it should close/hide the element;<field type="Color">
, try and navigate the page with your keyboard, when you focus a color-picker it should open its.color-dropdown
and allow you to navigate inside;<field type="Color" keywords="false">
delete the current value in the visible input first then try typing "inherit", "transparent" or "currentColor", it should set#000000
value;<field type="Color" keywords="navy,crimson..">
delete the current value in the visible input first then try typing "inherit", "transparent" or "currentColor", it should set a valid HEX/RGB(a)/HSL(a) value;<field type="Color">
which uses nokeywords
option. delete the current value in the visible input first then try typing "inherit", "transparent" or "currentColor", it should set that value;.color-dropdown
(where format is either "rgb" or "hsl"), focus one of them and try and change values by using the up/down buttons of the keyboard, it should instantly update the color picker on every change;.color-dropdown
(where format is either "rgb" or "hsl"), focus one of them and try and type 0-255 for "R", "G" or "B" for the<field type="Color" format="rgb">
field or 0-359 for "H", 0-100 for "S" or "L" for the<field type="Color" format="hsl">
, it should update the color picker on every change after a short delay (TinyColor will validate that color first);<field type="Color" format="hsl">
, if the "L" (Lightness component) is 100, the "S" (Saturation component) and the "H" (Hue component) both should be locked at 0, then decrease lightness to 99 and this will unlock Saturation control, ultimately if you set Lightness to less than 100 and Saturation to at least 1, this will ultimately unlock Hue control (this will be explained in the documentation).Testing the "onchange"
The implementation is similar with fancy-select, so you as tester, as well as third party devs should have a familiar feeling about color picker. For any of your
<field type="Color" control="advanced">
make sure to also test "onchange":Mobile Testing
.color-dropdown
and the mobile keyboard all are displayed on the mobile device screen; the color-picker will decrease its height by around 25% to fit most mobile devices;.color-dropdown
popup on taping outside the element itself;Accessibility Testing
Check the labels of all the visible inputs, find any ARIA related issues, report back anything you think it's not good enough in terms of accessibility, ARIA and so on.
The main focus:
<joomla-field-color-picker>
element is focused, hitEnter
/Space
to toggle the color picker visibilityTab
key, it should go to the next form fieldTab
key, it should navigate all focusable elementsActual result BEFORE applying this Pull Request
All
type="color"
fields that usecontrol="advanced"
are outdated in terms of accessibility, usability and jQuery dependency.Expected result AFTER applying this Pull Request
All your
type="Color"
fields that usecontrol="advanced"
are super awesome on any device and any user.Documentation Changes Required
This documentation section mentions nothing about the control property/setting of the
Color
field, the following is a draft to provide guidance for this Joomla form field type as coded inlibraries\src\Form\Field\ColorField.php
in Joomla 4.0. Some parts of it need to be updated by their developer or some Joomla intern knowledgeable of this particular form field.Documentation Draft
Provides a flexible form field that allow you to set a colour, with controls ranging from simple to advanced.
colors="#ff0000,#0000ff"
.keywords="initial,inherit,currentColor,transparent"
, however you can disable the feature by usingkeywords="false"
which will restrict the color picker to only use valid HEX, RGB(a) or HSL(a) values. If you include any of the default value (eg. "inherit") in the keywords list, it will remove the restriction.Note on HSL(a) Format and Advanced Control
TinyColor is the script behind the color picker of the "advanced" control. This script will understand that values like
hsl(0, 0%, 100%)
andhsl(150, 0%, 100%)
are visually identical, it will use thehsl(0, 0%, 100%)
value. In addition, if the L (Lightness component) is 100% then the S (Saturation component) and the H (Hue component) both should be locked at 0 in the inputs of the color picker; if you decrease Lightness to 99% it will unlock Saturation control, ultimately if you set Lightness to less than 100% and Saturation to at least 1%, this will unlock Hue control.Example XML Definition for Simple Control:
Example XML Definition for Slider Control: (this part need to be updated by its developer)
Example XML Definition for Advanced Control: