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
Implementing full associations for single weblink #349
Conversation
@infograf768 Thank you very much. |
@@ -200,6 +210,25 @@ public function getItem($pk = null) | |||
$registry->loadString($item->images); | |||
$item->images = $registry->toArray(); | |||
|
|||
// Load associated newsfeeds items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change newsfeeds to weblinks?
@@ -0,0 +1,61 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we minimize this file?
on ipad now. will look later. :) |
Yep, that's the only one. It was a mistake when we merged com_associations. |
I added the call into the manifest but not sure we need them. Even if that is a typo, we do not have an xtd-weblinks and I do not know if we need these js at all. For example, we do not have these js for newsfeeds. That type of js was introduced in joomla/joomla-cms#12561 and then modified. Calling @dgt41 to look into that. |
JHtml::addIncludePath(JPATH_COMPONENT . '/helpers/html'); | ||
JHtml::_('behavior.core'); | ||
JHtml::_('behavior.polyfill', array('event'), 'lt IE 9'); | ||
JHtml::_('script', 'com_weblinks/admin-weblinks-modal.js', array('version' => 'auto', 'relative' => true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the admin-weblinks-modal.js (currently we use not the min) because we use it here. If we not have this Javascript, it is not possible to select an item when creating a menu item for the single view.
Actually there should be a button for every type of content in Joomla (newsfeed, weblink, etc) |
OK. Will add the xtd-editor for weblinks and minimise js |
I guess we should be OK now. @zero-24 |
For me, almost everything works. I have only problems with the xtd plugin. But I think it's up to me. I'll try again tomorrow. |
@@ -116,9 +118,11 @@ COM_WEBLINKS_ORDER_HEADING="Order" | |||
COM_WEBLINKS_RIGHT="Right" | |||
COM_WEBLINKS_SAVE_SUCCESS="Web link successfully saved" | |||
COM_WEBLINKS_SEARCH_IN_TITLE="Search in title" | |||
COM_WEBLINKS_SELECT_A_WEBLINK="Select Weblink" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use "Web link" everywhere and not "Weblink"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. will do
Web Link maybe. will look elsewhere.
or web link in this phrase
defined('_JEXEC') or die; | ||
|
||
/** | ||
* Editor WEb Link button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web for WEb
concerning name of plugin, except for fields, they are singular https://github.com/joomla/joomla-cms/tree/staging/plugins/editors-xtd |
@astridx |
I have tested this PR successfully. Concerning name of plugin: I meant, that the names of all other weblink plugins (finder, system, search) are in singular. But that was only a suggestion. |
we have to normaise vs editors-xtd plugins, not any other type. 😃 |
@zero-24 |
fix some smal things
looks ok now from a quick review but I did not found time to test this yet. Sorry. |
I just noticed something. If an weblink is unpulished, you can see it in this single weblink view, if you know the link. This a security problem, right? |
I guess we need to debug this here: https://github.com/joomla-extensions/weblinks/blob/master/src/components/com_weblinks/models/weblink.php#L73-L83 |
btw, folks, we are freezing languages for the CMS on the 13th. |
It should throw a 404 when a single weblink menu item is clicked. |
this PR is ready for Test? |
It was, last time I tested. :) In June. |
can there be proper Test Instructions at first Comment? As far as i read there should be a "single weblink" (menue?) which i can't find, also no Button so i share Comments of @infograf768 in #346 why to have multlingual as you got to an Page after click on Weblink. |
as far as I remember, there is a already a single weblink menu item and with this patch instead of displaying directly the page targeted by this link, it displays a joomla frontend page containing the web link to click on, therefore permitting associations. |
You are right. The single view and the multilingual is not really neccessary for this component. But I think weblinks is a component where others (and I) have the opportunity to learn creating componentens.
Do I understand you right? You do not see the opportunity to create a single weblink view when you create a menu point? It should be similar to com_content, where you can create a menu item for a single article. |
I applied the path via Patch Tester and @franz-wohlkoenig is right, here I saw no menu item for a single web link. By the way, there was no JOOMLA/components/com_weblinks/views/weblink/tmpl/default.xml file in the folder. I think patch tester can not copy new files. And this file is new in this version. Then I applied the branch via git fetch upstream pull/349/head:weblinks_assoc and built a new pkg-weblinks-current.zip and installed this. After that I could create a new menu item. I saw the web link in the front end, when I clicked it. After publishing these web link I saw a 404 error. I could create web links and web links categories for other languages via multilingual component. And I saw the association tab in the web links component and I could use it. I made some checks on a website, that is not multilingual and there was everything OK, I mean, I saw no association tab. I had only one issue. I could not create a web link with the same alias in different languages. But I would like to merge this and open an issue for the opportunity to have the same alias for different languages. I tested with Joomla! 3.8.0-rc2-dev Development [ Amani ] 5-September-2017 14:00 GMT |
As title says.
See #346 (comment)
@zero-24
to be checked for unvoluntary modifications of already merged PRs
@astridx Please test