Skip to content

Comments

[4.0] Fixing lots of issues in Tags component and grouping toolbar buttons#23402

Merged
wilsonge merged 4 commits intojoomla:4.0-devfrom
Hackwar:j4tagsfixes
Jan 6, 2019
Merged

[4.0] Fixing lots of issues in Tags component and grouping toolbar buttons#23402
wilsonge merged 4 commits intojoomla:4.0-devfrom
Hackwar:j4tagsfixes

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Dec 31, 2018

This PR removes a lot of code in com_tags which is obsolete and refactors existing code to adhere to existing standards.

In TagsModel we have the checking() and getTable() method, which are not in use and where I would assume, that they have never been in use.

In the Tag View, I've cleaned up the code a bit to adhere to our existing codebase.

In the Tags View, I've noticed that we are constructing a level filter dropdown which then is never used in the output. We have this already covered with the filter form XML. I also did the grouping of the toolbar buttons that @bembelimen and @chmst have started.

Last but not least, I've changed the TagField, since that failed in my installation. I had an empty tag table and created one tag and after that it thought to give the parent_id to the field as value and do so as an integer, which the field handled by throwing errors that the value is not a string and not an array and that implode() fails miserably here and that again results in an SQL query error. Creating an array here solves that for now, but I can already tell that it wont be enough to make this work again.

Testing instructions

Simply play around with the component before and after applying the patch and notice that nothing really has changed, except for the crippling error when editing an existing tag, whcih should have gone away.

Issues that came up

Right now the dropdown does not work. I don't know what the issue is, but I'm unable to select a parent for a new tag. It always sticks to "none" and also throws a JS error on the console. @dgrammatiko Maybe you can have a look here?
Also, the ajax call to search for the new parent tag is dispatched to the frontend com_tags component. Somehow I feel very uneasy about this, that everybody can query the tags in the frontend unauthenticated and thus can create a complete list of all tags of the site by simply not handing over any filter params. From my perspective, this should be code in the backend and definitely authenticated and somehow restricted.
More to follow soon.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Dec 31, 2018
@dgrammatiko
Copy link
Contributor

Right now the dropdown does not work

Front end? Tag field?

@Hackwar
Copy link
Member Author

Hackwar commented Dec 31, 2018 via email

@chmst
Copy link
Contributor

chmst commented Dec 31, 2018

Looks good for me. But do not see the error in the tag field either.


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

@Hackwar
Copy link
Member Author

Hackwar commented Jan 2, 2019

Ok, explaining the issue with the tags field again in detail:

  • Current head of 4.0-dev with blog sample data and debugging enabled
  • Creating one tag to have a parent.
  • Creating a second tag to select as a child of the previous parent.
  • When creating that second tag, I can't select an existing tag as parent. The tags are retrieved and displayed, but I can't select them. I also get JS errors in the console. (FF64)
  • On a quick check, it seems as if the same issue is in the article edit view.

@dgrammatiko
Copy link
Contributor

but I can't select them

I am guessing it's from: #22263
That PR has numerous issues:

  • Custom elements cannot rely on other scripts being already loaded (this works right now because I'm lazy loading the custom elements, but you could load them asynchronously and then they'll fail)
  • It doesn't use mutation observers to mutate any added/removed options from the select (or the select itself)
  • It doesn't refresh correctly: in the article edit (admin) try to resize to mobile and observe the tags field, values disappear
  • Numerous accessibility issues mainly because choices itself is not accessible (and probably not fixable in Joomla's repo).

@Fedik can you please check those issues?

@Fedik
Copy link
Member

Fedik commented Jan 2, 2019

I don't know what the issue is, but I'm unable to select a parent for a new tag.

I just checked and it works fine for me in Chrome and Firefox, without errors in console.

screen 2019-01-02 14 18 07 644x339

@Hackwar which exactly error did you got?

@dgrammatiko

Custom elements cannot rely on other scripts being already loaded (this works right now because ...

If it works then all fine 😉

It doesn't use mutation observers...

because it does not need

It doesn't refresh correctly

The mobile styling issue I better leave to someone who better know the template styles 😉

choices itself is not accessible

hm? I thought it accessible enough, that why we pick that script

@Hackwar
Copy link
Member Author

Hackwar commented Jan 2, 2019

@Fedik TypeError: e.replace is not a function in choices.min.js

@dgrammatiko
Copy link
Contributor

If it works then all fine

Errrm, NOOOOO THIS IS WRONG and needs to be fixed!

because it does not need

False, this breaks on disconnect/reconnect, eg moving the element. Try to resize the page to get tabs to convert to accordion, then this is broken!

I thought it accessible enough

Well, it's not

@Fedik
Copy link
Member

Fedik commented Jan 2, 2019

@Hackwar sorry I cannot reproduce the error (I tried with a tags and an article).
Can you please try 2 thing:
1 update js, run npm install (if you did not made it before)
2 enable debug, and tell a full error trace (make screenshot or copy/paste here), it should show a correct line where error happened in choices.js instead of choices.min.js

@dgrammatiko

NOOOOO THIS IS WRONG and needs to be fixed!

but it is working 😏

False, this breaks on disconnect/reconnect, eg moving the element.

It does not fail because lack of mutation observer, it fail because the tab CE move a content of sections around (I do not ask why 😄), and so disconnectedCallback clean up the Choices instance.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 3, 2019

TypeError: html.replace is not a function[Weitere Informationen] choices.js:5921:11
stripHTML
http://localhost/j4tags/media/vendor/choicesjs/js/choices.js:5921:11
addItemText
http://localhost/j4tags/media/vendor/choicesjs/js/choices.js:152:46
_canAddItem
http://localhost/j4tags/media/vendor/choicesjs/js/choices.js:1456:79
_handleChoiceAction
http://localhost/j4tags/media/vendor/choicesjs/js/choices.js:1395:27
_onMouseDown
http://localhost/j4tags/media/vendor/choicesjs/js/choices.js:2047:12
_onMouseDown self-hosted:974:17

This is a copy of the trace of the problem. Even after pulling the latest changes and doing a clean npm i, I still get this error.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 3, 2019

All of these discussions about JS issues should however not prevent this PR from being merged.

@Fedik
Copy link
Member

Fedik commented Jan 4, 2019

@Hackwar by looking the trace, it seems that value not a string.
I have no idea why it happen in your case.
Can you try open a direct link for AJAX search for "green" in browser (if you have blog data, or for any other tag):
/index.php?option=com_tags&task=tags.searchAjax&like=green
in response you should get something like:

[{"value":"4","text":"Green","path":"green"},{"value":"5","text":"Green\/Lime","path":"green\/lime"}]

where "value":"4", is a string, and "value":4, would be an integer.

One thing I guessing, maybe you have different database version, and Joomla\CMS\Helper::searchTags returns an integer for you.

I have MySQL 5.7.24.

It not related to current pull request.

@Hackwar
Copy link
Member Author

Hackwar commented Jan 4, 2019

[{"value":2,"text":"test","path":"test"},{"value":3,"text":"test\/new","path":"test\/new"},{"value":4,"text":"test joomla","path":"test-joomla"}]
That is the response that I get. PHP 7.2.9, MariaDB 5.5.5

@Fedik
Copy link
Member

Fedik commented Jan 4, 2019

Thanks for checking. As I expected the type is not a string ("value":2, where 2 is integer)
MariaDB return ID as integer and MySQL as string, and the script expect a string.
I will check what can do with the script.

@chmst
Copy link
Contributor

chmst commented Jan 6, 2019

I have tested this item ✅ successfully on e66e9fd

I have tested this item successfully. The component behaves as expected from the users's poit of view.

For a test ist is necessary to add tags and therefore the patch #23395 mus be applied before testing.


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

@bembelimen
Copy link
Contributor

I have tested this item ✅ successfully on e66e9fd


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

@Quy
Copy link
Contributor

Quy commented Jan 6, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 6, 2019
@wilsonge wilsonge merged commit ec17681 into joomla:4.0-dev Jan 6, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 6, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 6, 2019
@wilsonge
Copy link
Contributor

wilsonge commented Jan 6, 2019

Thanks!

@Hackwar Hackwar deleted the j4tagsfixes branch April 27, 2019 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants