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

Use ajax power on module order field #11040

Merged
merged 7 commits into from
Sep 6, 2016
Merged

Use ajax power on module order field #11040

merged 7 commits into from
Sep 6, 2016

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jul 6, 2016

Enhance the current status quo

Try to create a new module in module manager. Select a position. Select another position
The dropdown ordering doesn't really represent the true order of the modules in the new position.
So my idea was to use ajax power to be always accurate on that dropdown.

Summary of Changes

Use ajax power, tight connection with the controller module.php

Preview

Mind the untranslated string!
No other module in that position:
screen shot 2016-07-06 at 23 23 04

With other modules in the same position:
screen shot 2016-07-06 at 23 24 48

Testing

Please DO NOT TEST this one!
Instead give a vote (up or down thumb) so PLT can make a decision (or PLT decides directly, whatever comes first)

"position": originalPos
},
success:function(response) {
console.log(response.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

debug code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is just to showcase some working code. it's not commit ready.
Eg $("#jform_position") the relation with the position field right now is hardcoded, needs an option in the XML for the field moduleorder

@Devportobello
Copy link
Contributor

Dunno if people use the ordering directly in edit mode, but this is an improvment when there is many module in same position

@dgrammatiko
Copy link
Contributor Author

@Devportobello from a UX perspective this is the right approach.
Now is this something that people actually use? I have no clue...

@RonakParmar
Copy link

I have tested this item 🔴 unsuccessfully on 4885e07


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

@RonakParmar
Copy link

I have applied this PR and try to create "custom" module, I have jQuery error in console.
Joomla Version: Joomla! 3.6.0-rc Release Candidate [ Noether ] 28-June-2016 20:34 GMT
Tested in Ubuntu 12.04, FireFox 47.0.1.screen shot 2016-07-12 at 07 15 42


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

@killoltailored
Copy link

I have tested this item 🔴 unsuccessfully on 4885e07


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

@killoltailored
Copy link

I have tested after applying patch with Joomla Version Joomla! 3.6.0-rc Release Candidate [ Noether ] 28-June-2016 20:34 GMT in Windows 8.1, Firefox 47.0.1
Create new custom module and found jQuery error. Test with both case select unused position and used position.screen shot 2016-07-12 at 07 27 03screen shot 2016-07-12 at 07 27 06


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

@dgrammatiko
Copy link
Contributor Author

@RonakParmar @killoltailored Thanks for testing but this is RFC (Request for comments) which means a decision is needed here before I spent more time on this pr. So an up vote or down vote on the scope is enough at the moment (even if the code is not show time ready). Thanks again

@brianteeman
Copy link
Contributor

brianteeman commented Jul 12, 2016 via email

@dgrammatiko
Copy link
Contributor Author

@brianteeman I thought that RFC indicator was enough. Maybe I should have opened this as an issue without code attached so people wouldn't be able to do any testing.

@brianteeman
Copy link
Contributor

brianteeman commented Jul 12, 2016 via email

@gunjanpatel
Copy link
Contributor

I am sure they are testing it because they see pending status of the task. It is hard to understand what is RFC from the title of the task. 😄

It isn't should be in a "Needs Review" status? Or may just "New" status and which waiting to be "Confirmed".

Thanks.


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

@brianteeman
Copy link
Contributor

brianteeman commented Jul 14, 2016 via email

@JoshJourney
Copy link

Great idea. So who are you asking if this is worth it before working on this PR? The folks who manage PR's for new releases of Joomla? Testers?

@brianteeman
Copy link
Contributor

@dgt41 - seems the consensus is that yes this is a good idea but that the code currently doesnt work yet


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

- Seperate html-javascript (no inline scripts)
- Fix broken ajax
- Fix wrong escaping
- Introduce option for the linked field
- No new strings
@dgrammatiko
Copy link
Contributor Author

Now it should be testable

@gunjanpatel
Copy link
Contributor

Can you remove RFC from title then?

@dgrammatiko dgrammatiko changed the title RFC Use ajax power on module order field Use ajax power on module order field Aug 18, 2016
@brianteeman
Copy link
Contributor

Not sure if it is doing what I expect it to do
After applying the patch the ordering in the select is correct but when viewing it on the front end the ordering selected is not being used


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

@dgrammatiko
Copy link
Contributor Author

@brianteeman before applying this patch was the behaviour different? All this pr is doing is reloading the ordering when the position is changed, it shouldn't interfere with the actual ordering in the front end...

*
* @note The field does not include an upload mechanism.
* @see JFormFieldMedia
* @since 11.1
Copy link
Contributor

Choose a reason for hiding this comment

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

11.1? better always to use __DEPLOY_VERSION__

Copy link
Contributor

Choose a reason for hiding this comment

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

I think __DEPLOY_VERSION__ needs to be on other places too where 3.7 is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrepereiradasilva @gunjanpatel you are both right but I think __DEPLOY_VERSION__ came after july 6th. Anyways I will fix this..

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh sorry didn't noticed dates. Thanks 👍

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 9ed547e

No idea what the issue was I saw before as I cant replicate it now

I still dont really see what the difference is now though compared to before this pr


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

@AnishaTailored
Copy link

I have tested this item ✅ successfully on 9ed547e

I have tested this successfully. If we change the position then the ordering field fetches modules of the newly selected position.
But shouldn't Ordering dropdown be jQuery chosen dropdown as it was before this PR.


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

@zero-24
Copy link
Contributor

zero-24 commented Sep 5, 2016

@dgt41 can you double check the since tags? If that is done i think we can RTC

@dgrammatiko
Copy link
Contributor Author

@zero-24 there you go!

*
* @return string The field input markup.
*
* @since 11.1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getInput() was there before so the since should not change. (11.1 is wrong should be 1.6)

@zero-24
Copy link
Contributor

zero-24 commented Sep 5, 2016

Hmm i have found two more ;)

@zero-24
Copy link
Contributor

zero-24 commented Sep 5, 2016

Go for it @joomla-cms-bot please make us happy 😄


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 5, 2016
@rdeutz rdeutz added this to the Joomla 3.6.3 milestone Sep 6, 2016
@rdeutz rdeutz merged commit 0b9b3a8 into joomla:staging Sep 6, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 6, 2016
@dgrammatiko dgrammatiko deleted the rfcModuleOrder branch September 8, 2016 12:50
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