Skip to content
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

[com_modules] modules view (modal) - Implement searchtools #9232

Merged
merged 20 commits into from
Apr 14, 2016

Conversation

andrepereiradasilva
Copy link
Contributor

Description

This PR adds searchtools to com_modules modal layout modules view.

Also, in normal layout:

  • improves responsive visual (mobile/tablet/desktop)
  • the acess column now orders by access level designation and not by access level id (it doesn't make a lot of sense order by the access level ID ... but makes sense order by the acess level DESIGNATION).
  • code improvements
Before PR (modal)

before-pr

After PR (modal)

after-pr

How to test

Normal view:

  1. Install latest staging and apply this patch
  2. Go to Extensions -> Modules
  3. Check if search, filters, ordering and pagination are working properly
  4. Test on mobile and tablet (ex: reduce browser width)

Modal view (test in frontend and backend):

  1. Go to any content and click on "+Module" button on the editor to open the modal layout
  2. Check if search, filters, ordering and pagination are working properly.
  3. Test selecting a position or a module.

Observations

  1. Changed the modal default list when in the backend, now also shows other states (trashed, archved and unpublished). Why? To show the same states as "+Article" modal - consistency. In the frontend only shows enabled modules in the current language or in All language.
  2. Added a new /components/com_modules/models/forms/filter_modules.xml file to control the frontend searchtools fields (a similiar one already exists for the com_content in /components/com_content/models/forms/filter_articles.xml). Please tell if there are any problems with this.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on ba84682

Tested successfully - I still see zero value in the style box


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

@andrepereiradasilva
Copy link
Contributor Author

I still see zero value in the style box

sorry, didn't understand your comment.

@brianteeman
Copy link
Contributor

Dont worry about it - just personally dont see the need to add a style here
as I think it is better to be done in the module and just confuses the UI
here with a function right at the top that is not easy to understand or
even that most people will even have a clue what it is for

On 28 February 2016 at 13:06, andrepereiradasilva notifications@github.com
wrote:

I still see zero value in the style box

sorry, didn't understand your comment.


Reply to this email directly or view it on GitHub
#9232 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

window.parent.jInsertEditorText("{loadmodule " + type + "," + name + extraVal + "}", "' . $editor . '");
window.parent.jModalClose();
};
modulePosIns = function(position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add back the 2 tabs in this javascript part so the rendered output is more aligned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do when i fixed merge conflicts

BTW, just for info, i tried a new javasctipt to add to core.js, it worked good but didn't want to merge in this PR

    /**
     * Insert a loadmodule or loadposiiton text shortcut in modules modal windows. Used in modules modal.
     * USED IN: administrator/components/com_modules/views/modules/tmpl/modal.php
     *
     * @param   string   component   The component in usage (ex: com_content).
     * @param   string   type        The type of insert (module or position).
     * @param   string   what        What to insert, the position or module type.
     * @param   string   title       The title.
     *
     * @return  boolean  False to remove the default click event.
     */
    Joomla.insertEditorShortCode = function(component, type, what, title) {
        var editor = document.getElementById('editor').value, extraClass = document.getElementById('extra_class').value;
        if (component === 'com_modules')
        {
            window.parent.jInsertEditorText('{load' + type +' ' + what + ((title !== '') ? ',' + title : '') + ((extraClass !== '') ? ',' + extraClass : '') + '}', editor);
            window.parent.jModalClose();
        }
        return false;
    }

then call it with

For modules:
onclick="Joomla.insertEditorShortCode('com_modules', 'module', '<?php echo $this->escape($item->module); ?>', '<?php echo $this->escape($item->title); ?>');">

For position:
onclick="Joomla.insertEditorShortCode('com_modules', 'position', '<?php echo $this->escape($item->position); ?>', '');"

What to you think? for a future PR after this is merged...

also has the advantage that is vanilla js

And of course, could also be modified to use data-* attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can have vanilla js here:

moduleIns = function(type, name) {
    var extraVal ,fieldExtra = document.getElementById("extra_class");
    extraVal = (fieldExtra.length && fieldExtra.value.length) ? "," + fieldExtra.value : "";
    window.parent.jInsertEditorText("{loadmodule " + type + "," + name + extraVal + "}", "' . $editor . '");
    window.parent.jModalClose();

I wouldn't abstract the function in core.js as it makes it harder to trace back...
On the other hand that way we discard the inline script, so I see an advantage there as well. You decide here

@anibalsanchez
Copy link
Contributor

I have tested this item ✅ successfully on ba84682

Test OK


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

@brianteeman
Copy link
Contributor

RTC thanks


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 29, 2016
@shubhamnba2009
Copy link

Hi, i want to contribute by testing this PR.
I have a ques and this may seem silly to some. What does the term latest staging means?

@alikon
Copy link
Contributor

alikon commented Mar 2, 2016

Means the latest "version" on the current branch "staging" it contains all
pr merged
On 2 Mar 2016 8:39 am, "Shubham Rajput" notifications@github.com wrote:

Hi, i want to contribute by testing this PR.
I have a ques and this may seem silly to some. What does the term latest
staging means?


Reply to this email directly or view it on GitHub.

@anibalsanchez
Copy link
Contributor

The term 'staging' refers to something that's staged to be delivered.

In Joomla context, it is the name of all the latest accepted changes.

Then, to test a new change, you have to download from Github the latest
staging (click in the main repo Download button) and install that version
for testing.

Additionally, some PRs provide the whole Joomla to avoid the hassle of
patching.

Happy testing !
On Mar 2, 2016 8:39 AM, "Shubham Rajput" notifications@github.com wrote:

Hi, i want to contribute by testing this PR.
I have a ques and this may seem silly to some. What does the term latest
staging means?


Reply to this email directly or view it on GitHub
#9232 (comment).

@shubhamnba2009
Copy link

Just one more small doubt. I have installed joomla version 3.4.8 and using xampp to run it.
So lnstalling latest versiofromn means I have to download latest version github then remove 3.4.8 version and then install latest version of joomla? Is this the procedure or I can upgrade the 3.4.8 version without removing it? Thanks in advance

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @anibalsanchez, @brianteeman


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

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @anibalsanchez, @brianteeman


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

@andrepereiradasilva
Copy link
Contributor Author

ignore again the unit tests label, somehow my git changed the filename of the xcache unit test, i reverted.
Seems my git has some issues with the naming of that file, maybe because is windows...

Conflicts fixed. @brianteeman please remove again the label sorry.

@rdeutz
Copy link
Contributor

rdeutz commented Apr 13, 2016

Two questions:

  1. Why was the type column removed?
  2. Because it changes the interface, shouldn't this go into 3.6.x?

@brianteeman
Copy link
Contributor

Dont know abut the removal of the type column (perhaps a mistake I only noticed the removal of the pages column which was meaningless) but this was only a "feature" added in 3.5.0 and to me its a bug fix/improvement

@andrepereiradasilva
Copy link
Contributor Author

the type column is there is only hidden in tablet view.

@brianteeman
Copy link
Contributor

As per your screenshot in the first post and a retest I just did right now which shows the same as your screenshot.
Type IS there in the search tools but is NOT there in the displayed columns which as shown in your own screenshots was there before

@rdeutz
Copy link
Contributor

rdeutz commented Apr 13, 2016

@brianteeman what was added in 3.5.0?

@brianteeman
Copy link
Contributor

@rdeutz this a new plugin not present util 3.5.0 #8064

@rdeutz
Copy link
Contributor

rdeutz commented Apr 13, 2016

funny I have testet it, I am getting old :-), thanks

@andrepereiradasilva
Copy link
Contributor Author

ok will check the type column, maybe the popup is a tablet size (width) and so it doesn't appear.

@andrepereiradasilva
Copy link
Contributor Author

just tested, yes the popup is tablet size, i will change it.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @anibalsanchez, @brianteeman


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

@andrepereiradasilva
Copy link
Contributor Author

Done.

BTW not related to this PR, but the popups width shouldn't be fixed!

@brianteeman
Copy link
Contributor

All good to me - will let @rdeutz decide on 3.5.2 or 3.6


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

@rdeutz
Copy link
Contributor

rdeutz commented Apr 13, 2016

I think this here is edge case and I plan to merge it into 3.5.2, because #9208, #9206, #9198 fixing bugs and should be merged in 3.5.2 and this one here would make the interface more consistent

@brianteeman
Copy link
Contributor

thanks

On 13 April 2016 at 17:28, Robert Deutz notifications@github.com wrote:

I think this here is edge case and I plan to merge it into 3.5.2, because
#9208 #9208, #9206
#9206, #9198
#9198 fixing bugs and should
be merged in 3.5.2 and this one here would make the interface more
consistent


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9232 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@andrepereiradasilva
Copy link
Contributor Author

ok will fix the merge conflicts in the other 3 PR when i have time.

@rdeutz rdeutz merged commit 4cdf71a into joomla:staging Apr 14, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 14, 2016
@andrepereiradasilva andrepereiradasilva deleted the com_modules-searchtools branch April 14, 2016 07:13
@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016
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.

None yet

9 participants