Skip to content

[4.2] Use layouts in color field#36553

Closed
laoneo wants to merge 14 commits intojoomla:4.2-devfrom
Digital-Peak:j4/fields/color-layout
Closed

[4.2] Use layouts in color field#36553
laoneo wants to merge 14 commits intojoomla:4.2-devfrom
Digital-Peak:j4/fields/color-layout

Conversation

@laoneo
Copy link
Copy Markdown
Member

@laoneo laoneo commented Jan 3, 2022

Followup Pull Request for pr #36551 and issue #36548.

Summary of Changes

Adds the layout filter for the color custom field.

Additionally it deprecates the control parameter in favor of the layout one in the color filed to be more inline with the other core form fields.

This should be tested after #36551 is merged.

Testing Instructions

  • Create a color custom field
  • Change the form layout to slider or simple in the options tab
  • Edit an article

Actual result BEFORE applying this Pull Request

Color field is always displayed with the same result.

Expected result AFTER applying this Pull Request

Color field is displayed with different layouts.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.1-dev labels Jan 3, 2022
@laoneo laoneo marked this pull request as ready for review January 6, 2022 09:04
@laoneo laoneo requested a review from chmst as a code owner January 6, 2022 09:04
@crystalenka
Copy link
Copy Markdown
Member

I have tested this item 🔴 unsuccessfully on 59b08bb

All the layout options work except for the slider. For the slider, it appears, but it doesn't show the hue as expected. I'll add a screenshot in the next comment.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36553.

@crystalenka
Copy link
Copy Markdown
Member

crystalenka commented Jan 9, 2022

Slider screenshot. There's no color behind it so it looks empty.
Screen Shot 2022-01-09 at 12 30 35


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36553.

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Jan 17, 2022

Even as I didn't change something on the JS side, can you rebuild it with npm or download a new build from here? Because on my dev server it worked after a npm ci.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jan 18, 2022

It is reproducible with the prebuilt package.

36553

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jan 18, 2022

The linear-gradient values are different.

Fields:
<input type="range" min="0" max="360" class="form-control color-slider" id="hue-slider" data-type="hue" style="background: rgba(0, 0, 0, 0) linear-gradient(90deg, rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255), rgb(255, 255, 255)) repeat scroll 0% 0%; appearance: none;">

Administrator Template Styles:
<input type="range" min="0" max="360" class="form-control color-slider" id="hue-slider" data-type="hue" style="background: rgba(0, 0, 0, 0) linear-gradient(90deg, rgb(83, 19, 19), rgb(83, 19, 19), rgb(83, 38, 19), rgb(83, 57, 19), rgb(83, 77, 19), rgb(70, 83, 19), rgb(51, 83, 19), rgb(32, 83, 19), rgb(19, 83, 25), rgb(19, 83, 45), rgb(19, 83, 64), rgb(19, 83, 83), rgb(19, 64, 83), rgb(19, 45, 83), rgb(19, 25, 83), rgb(32, 19, 83), rgb(51, 19, 83), rgb(70, 19, 83), rgb(83, 19, 77), rgb(83, 19, 57), rgb(83, 19, 38), rgb(83, 19, 19), rgb(83, 19, 19)) repeat scroll 0% 0%; appearance: none;">

@laoneo laoneo added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 10, 2022
@laoneo laoneo changed the base branch from 4.1-dev to 4.2-dev April 8, 2022 16:14
@laoneo laoneo requested a review from rdeutz as a code owner April 8, 2022 16:14
@laoneo laoneo changed the title [4.1] Use layouts in color field [4.2] Use layouts in color field Apr 8, 2022
@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Apr 8, 2022

Found the issue, can you guys test again? Ignore the conflict, will disappear when 4.2 got the commits from 4.1 merged.

@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Apr 13, 2022
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Apr 13, 2022

Before making a selection:
color-slider-pre

After making a selection and saving:
color-slider-post

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Apr 21, 2022

The slider shows the colors based on the initial color. So when you change it the range changes. Honestly I'm not much into this, perhaps @dgrammatiko can shed here some light why this happens. It is the same when you give it a default color.

@brianteeman
Copy link
Copy Markdown
Contributor

@laoneo I think you are missing what @Quy is saying.

image

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Apr 21, 2022

Don't think so as the color range of the slider is based on the available color. So when you choose one, the range changes. So what I do not understand is how to have a fixed color range which works in all situations.

@dgrammatiko
Copy link
Copy Markdown
Contributor

perhaps @dgrammatiko can shed here some light why this happens.

I need to debug it but it seems that the removal of the $control is not right. In the advanced this var controls which type of value should be accepted/outputed (but might be wrong, this is just by reading the code).

About the custom fields and particularly the slider version: it will never work correctly as the JS and the HTML have both IDs hardcoded and you can't really have one ID for multiple fields, will never work (not something you did here and also I had criticised that when this field was introduced but as usual I was ignored)

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Apr 21, 2022

Would be great if you can help me out here.

but as usual I was ignored

Your removed jQuery from core, what did you expect then 🤣

@dgrammatiko
Copy link
Copy Markdown
Contributor

Your removed jQuery from core, what did you expect then

And that's my punishment for keeping the project somewhat competitive, fair enough 🙄

Joke aside, I'm kinda busy today, I'll try to sneak some time this weekend, unless someone is faster

@laoneo laoneo closed this May 13, 2022
@laoneo laoneo deleted the j4/fields/color-layout branch May 13, 2022 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants