Skip to content

[4.0] minicolors#27829

Merged
wilsonge merged 1 commit intojoomla:4.0-devfrom
brianteeman:minicolors
Apr 11, 2020
Merged

[4.0] minicolors#27829
wilsonge merged 1 commit intojoomla:4.0-devfrom
brianteeman:minicolors

Conversation

@brianteeman
Copy link
Contributor

This PR ensures that the position of the colorpicker is positioned correctly by increasing the specificity of the css AND making sure it is actually imported. It also ensures that the pointer is used and not the I-beam

PR for #27538
npm -i as it is a css change

Further details in the report #27538

This PR ensures that the position of the colorpicker is positioned correctly by increasing the specificity of the css AND making sure it is actually imported. It also ensures that the pointer is used and not the I-beam

PR for joomla#27538
npm -i as it is a css change
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 6, 2020
@C-Lodder
Copy link
Member

C-Lodder commented Feb 6, 2020

The CSS file is already being loaded here: https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/form/field/color/advanced.php#L76

@brianteeman
Copy link
Contributor Author

thats the vendor supplied css not the override

@C-Lodder
Copy link
Member

C-Lodder commented Feb 6, 2020

ah my bad

@import "blocks/global"; // Leave this first

// jQuery Minicolors
@import "../../../../build/media_source/system/scss/jquery-minicolors";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong! There is already a css file so either override it or extend it. The template shouldn't be aware of every 3rd pd extension that might be used in some pages, Joomla has specific layouts for these (input colour advanced for this one iirc)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why is the file there and why is it used in Cassiopeia?

Getting really sick of you telling everyone they are wrong without contributing. You're starting to be a troll.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting really sick of you telling everyone they are wrong without contributing

This is funny I did #26077 so people can follow up and fix the rest and now you're undoing it instead of fixing whatever bug. Do you expect me to fix this ugly spaghetti of css alone?

So why is the file there and why is it used in Cassiopeia?

If it's embedded in the template's css then it's wrong. Joomla is modularised by default making it a freaking monolith is wrong, but hey it's working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you missed the fact that this PR was to fix an issue raised by someone with far more experience than me at scss.

I guess you missed the fact that I specifically asked where to put the code and the same person with far more experience than me at scss told me where to put it. Which is exactly what I did.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoever told you to put the css there IS WRONG. Joomla by default is modular, with this kind of code snippets you're making it monolith. Prove me wrong!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be quite clear here: I'm arguing with this approach because you're effectively undoing #26077

@jwaisner
Copy link
Member

jwaisner commented Feb 6, 2020

I have tested this item ✅ successfully on 947d88d


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

@joomla-cms-bot joomla-cms-bot removed NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Apr 7, 2020
@richard67
Copy link
Member

I have tested this item ✅ successfully on 947d88d


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 11, 2020
@richard67 richard67 added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Apr 11, 2020
@dgrammatiko
Copy link
Contributor

@richard67 RTC? Really??? Did you ready any of my comments?

@richard67
Copy link
Member

@dgrammatiko Yes I did read them but I'm not deep enough into these things to tell what is right and what is wrong. I only can see this PR solves an issue.

@wilsonge Should I revert the RTC until the discussion is clarified?

@richard67
Copy link
Member

@C-Lodder What's your opinion?

@wilsonge wilsonge merged commit ffb0aea into joomla:4.0-dev Apr 11, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 11, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Apr 11, 2020

This will do. It can be followed up with a vendor specific override in a future PR

@wilsonge wilsonge added this to the Joomla 4.0 milestone Apr 11, 2020
@brianteeman
Copy link
Contributor Author

thanks <200 now

@brianteeman brianteeman deleted the minicolors branch April 11, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants