Skip to content

Comments

[4.0] Replace chosen.js to choice.js#22263

Merged
wilsonge merged 43 commits intojoomla:4.0-devfrom
Fedik:choice-js
Nov 11, 2018
Merged

[4.0] Replace chosen.js to choice.js#22263
wilsonge merged 43 commits intojoomla:4.0-devfrom
Fedik:choice-js

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Sep 19, 2018

Summary of Changes

This a try to to replace chosen.js to choice.js.

  • Module position field
  • Tags field
  • Category field
  • Filter fields

@wilsonge is any else field is missed in the list?

Testing:

Module editing: create custom position, search existing positions
Article editing: test Tag field, test category field
Articles filter: test the filter by multiple category, Tag, etc,

Everything should work as before the patch.

Documentation Changes Required

maybe

@brianteeman
Copy link
Contributor

ooh getting excited - on my to do list for tomorrow

type="moduleposition"
<field
name="position"
type="ModulesPosition"
Copy link
Contributor

Choose a reason for hiding this comment

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

didnt we have an issue with case in the xml files before?

Copy link
Member Author

Choose a reason for hiding this comment

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

it because class autoloader,
if I keep modulesposition it will try to search for ModulespositionField.php but actual file name is ModulesPositionField.php

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, there was an issue about it, but cannot find

"licenseFilename": "LICENSE"
},
"choices.js": {
"name": "choices.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

This need to be changed to just choices otherwise you get this error in the build

Error: ENOENT: no such file or directory, open 'C:\htdocs\40\media\vendor\choices.min.js\css\choices.css'

Copy link
Member Author

@Fedik Fedik Sep 20, 2018

Choose a reason for hiding this comment

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

That what I am had afraid of. Currently I am working with "dev" mode and on on Linux, so all works fine.
The problem that the package name is choices.js https://github.com/jshjohnson/Choices/blob/master/package.json

This will be need to find a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the problem is with our build scripts and need to be solved there or it will happen again with another package (I'm still new to npm)

Copy link
Contributor

@wilsonge wilsonge Sep 23, 2018

Choose a reason for hiding this comment

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

You should be ok changing the name on this line to choices. The key on line 124 is the path to the node_modules dir

Copy link
Member Author

@Fedik Fedik Sep 23, 2018

Choose a reason for hiding this comment

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

The key on line 124 is the path to the node_modules dir

@wilsonge well, no, I thought the same, I tried, and that not work.
but I think it is fine not keep an original name,
just have to find out why an Install task fail on Windows

I think I have fixed Error: ENOENT: no such file or directory,, there was incorrect replacing of the file extension.
But @brianteeman said there now some empty CSS file, after build, and I do not have a windows to check in details.

I have looked in my windows but there only a trees 😄

Copy link
Contributor

@wilsonge wilsonge Sep 23, 2018

Choose a reason for hiding this comment

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

image

Well that definitely still builds ok and drops the .js in the folder path. I don't have windows either and Rowan who's my go to is on holiday at the moment. @brianteeman can I try sorting out a screenshare with you at some point this week after work?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, okay, then I missed something 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

@wilsonge of course just name a time/date

Copy link
Contributor

Choose a reason for hiding this comment

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

@wilsonge @Fedik the name part in the json is supposed to set a custom name for the vendor. IIRC there are couple of crappy names in the dependencies with @ or other strange .something parts in the names. If the code is not doing what is supposed to do just blame it on me but fwiw it was implemented based on some specific cases...

Copy link
Member Author

Choose a reason for hiding this comment

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

I have renamed to choicesjs

$attr .= $required ? ' required' : '';
$attr .= $autofocus ? ' autofocus' : '';

// To avoid user's confusion, readonly="true" should imply disabled="true".
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to disabled="disabled"?

?>

<joomla-field-categoryedit><?php
echo implode($html);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it one line <?php echo implode($html); ?>

Copy link
Member Author

Choose a reason for hiding this comment

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

you are looking in wrong commit 😃


Text::script('JGLOBAL_SELECT_NO_RESULTS_MATCH');
Text::script('JGLOBAL_SELECT_PRESS_TO_SELECT');
//Text::script('JGLOBAL_TYPE_OR_SELECT_SOME_OPTIONS');
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

it not finished, anything can happen :neckbeard:

brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Oct 18, 2018
### The problem
when running the scripts with joomla#22263 they were trying to process the choices css files as if they were js.
this was only present on windows

### The solution??
hopefully this is correct. It works for me but anything with regex and js and I am stabbing in the dark
@brianteeman
Copy link
Contributor

see #22706 for the corrected(?) build script

wilsonge pushed a commit that referenced this pull request Oct 19, 2018
### The problem
when running the scripts with #22263 they were trying to process the choices css files as if they were js.
this was only present on windows

### The solution??
hopefully this is correct. It works for me but anything with regex and js and I am stabbing in the dark
@Fedik
Copy link
Member Author

Fedik commented Oct 19, 2018

I will check this weekend, also the conflicts

@brianteeman
Copy link
Contributor

I "think" the error is related to the calendar field

 Conflicts:
	administrator/components/com_categories/Field/CategoryeditField.php
	administrator/components/com_modules/tmpl/module/edit_positions.php
	build/build-modules-js/update.js
@Fedik
Copy link
Member Author

Fedik commented Oct 19, 2018

I am getting this error when opening an article with some fields

@brianteeman please tell me which field types do you have in that article?

@Fedik
Copy link
Member Author

Fedik commented Oct 19, 2018

@laoneo do you remember what this thing is doing?

$xml = new \DOMDocument;
$xml->loadHTML($formField->__get('input'));
$options = $xml->getElementsByTagName('option');

the warning because DOMDocument not know about a custom tags, hm

@brianteeman
Copy link
Contributor

There were two fields. Calendar and text

@Fedik
Copy link
Member Author

Fedik commented Oct 20, 2018

@brianteeman should work now

// Choose the first category available
$xml = new \DOMDocument;
libxml_use_internal_errors(true);
$xml->loadHTML($formField->__get('input'));
Copy link
Member

Choose a reason for hiding this comment

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

Sry if its already answered but what happens here that this hack is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

see #22263 (comment)

DOMDocument shows a warning on unknown elements such as html5 and custom elements

Copy link
Member

Choose a reason for hiding this comment

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

Ok thx

@brianteeman
Copy link
Contributor

@Fedik yes that problem has been resolved - thanks

@wilsonge
Copy link
Contributor

So just to be clear is this PR now working (with a bit of a grim hack to the DomDocument stuff?)

@Fedik
Copy link
Member Author

Fedik commented Oct 24, 2018

for me, working :)

@wilsonge wilsonge merged commit 141a5e3 into joomla:4.0-dev Nov 11, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Nov 11, 2018
@wilsonge
Copy link
Contributor

Thankyou for this!

@infograf768
Copy link
Member

As stated in #23289, choices.css is loaded after template.css, therefore overriding specific overrides in template.css

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants