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

Change selector for input order field #11571

Closed
wants to merge 1 commit into from
Closed

Change selector for input order field #11571

wants to merge 1 commit into from

Conversation

nikosdion
Copy link
Contributor

Summary of Changes

Commit 93f90ef had the exact opposite
effect of what was advertised, i.e. it made it impossible to use
visible ordering input fields when using the drag'n'drop reordering.
This has broken –among other things– FOF 2.x which is part of core
Joomla!. It's worth noting that BEFORE this commit it was actually
possible to have visible reordering fields and I was in fact using them
in FOF 2 (which is included in Joomla! itself) since 2014. So, basically,
you guys broke Joomla! because you didn't test that the commit does
what it says it does instead of what the commiter thought his commit
does. TEST. BEFORE. COMMITING!!!

Interestingly, the actual problem @sebastienheraud was trying to solve
was in fact solved by the changes @alexvanniel did in gh-5731 This would
have been all too well if gh-5731 hadn't kept the :hidden specifiers
from commit 93f90ef which had introduced
a regression.

My changes keep the ACTUAL problem's fix from gh-5731 and remove the
regression introduced in 93f90ef

Testing Instructions

I am not providing any on the grounds that you blindly accepted commit
93f90ef and gh-5731 without testing.

Since this fixes FOF 2 which is part of Joomla! I expect you to merge it
anyway.

For what it's worth, I have tested this PR with all of my components using
both FOF 2 and FOF 3. Before the PR drag'n'drop reordering with visible order
fields was impossible: the row would move but the value in the ordering field
remained the same. After the PR drag'n'drop reordering with visible order works
as expected: the row would move AND the value in the ordering field changed.

I also tested with com_content. Before AND after the change you can drag'n'drop
reorder just fine. PLEASE DO TEST WITH CORE COMPONENTS YOURSELVES! We
must be sure that no core component breaks with this change. Also, please do
test with third party components for the same reason. Let's do this the right way.

Documentation Changes Required

None. This PR fixes what you guys had already broken.

#### Summary of Changes

Commit 93f90ef had the exact opposite
effect of what was advertised, i.e. it made it **impossible** to use
visible ordering input fields when using the drag'n'drop reordering.
This has broken –among other things– FOF 2.x which is part of core
Joomla!. It's worth noting that BEFORE this commit it was actually
possible to have visible reordering fields and I was in fact using them
in FOF 2 (which is included in Joomla! itself) since 2014. So, basically,
you guys broke Joomla! because you didn't test that the commit does
what it says it does instead of what the commiter thought his commit
does. TEST. BEFORE. COMMITING!!!

Interestingly, the _actual_ problem @sebastienheraud was trying to solve
was in fact solved by the changes @alexvanniel did in gh-5731  This would
have been all too well if gh-5731 hadn't kept the `:hidden` specifiers
from commit 93f90ef which had introduced
a regression.

My changes keep the ACTUAL problem's fix from gh-5731 and remove the
regression introduced in 93f90ef

#### Testing Instructions

I am not providing any on the grounds that you blindly accepted commit
93f90ef and gh-5731 without testing.

Since this fixes FOF 2 which is part of Joomla! I expect you to merge it
anyway.

For what it's worth, I have tested this PR with all of my components using
both FOF 2 and FOF 3. Before the PR drag'n'drop reordering with visible order
fields was impossible: the row would move but the value in the ordering field
remained the same. After the PR drag'n'drop reordering with visible order works
as expected: the row would move AND the value in the ordering field changed.

I also tested with com_content. Before AND after the change you can drag'n'drop
reorder just fine. **PLEASE DO TEST WITH CORE COMPONENTS YOURSELVES!** We
must be sure that no core component breaks with this change. Also, please do
test with third party components for the same reason. Let's do this the right way.

#### Documentation Changes Required

None. This PR fixes what you guys had already broken.
@brianteeman
Copy link
Contributor

Can't test so won't test

@nikosdion
Copy link
Contributor Author

@brianteeman You CAN test it. You can use the FOF 2.x demo component (todo) from https://github.com/akeeba/todo-fof-example/tree/master create a bunch of todo items and try reordering them.

However, I expected you to set it to RTC and commit it without any further ado like you did with the TWO commits that completely screwed up drag and drop reordering. You know, the ones I wasted two hours of my time to track down and fix in this PR because you guys don't care when you're breaking backwards compatibility and only play it hard when someone WHO ACTUALLY KNOWS HOW THIS THING WORKS spends his time fixing what you broke!

Forget it. I'll just remove the feature you stupidly broke from FOF 3 and let you figure out what to do with the end of life FOF 2 you still have in the core. I won't try to fix core bugs again. It's clear now that you will only blindly accept code from idiots who don't know how Joomla! works and break it instead of the people who DO know how it works and try to fix it. My bad. I should have learned by now.

@nikosdion nikosdion closed this Aug 12, 2016
@nikosdion nikosdion deleted the fix/dnd-without-hidden-fields branch August 12, 2016 16:28
@rdeutz
Copy link
Contributor

rdeutz commented Aug 12, 2016

What's going on here, I am just looking how I can test it.

@brianteeman
Copy link
Contributor

@nikosdion I didn't test, merge or mark rtc either of the two pr you refer to.

@ggppdk
Copy link
Contributor

ggppdk commented Aug 12, 2016

I had noticed it did not work with order input fields visible,
and the selector seem strange,

  • but did not look at it any more at that particular time

Tested this fix and it works:


To test with Joomla backend featured manager

  1. isis template add file css/custom.css with:
.text-area-order, .sortable-handler + input {
  display: inline !important;
  width: 40px; text-align: right;
}
  1. In feature manager make hard refresh to get new JS loaded and sort by the ordering the column
  2. E.g. i have 10 records, manually (typing) reverse the last 4 items e.g. 7 - 8 - 9 - 10 to 10 - 9 - 8 - 7
  3. Drag and drop some of the records at the top, this will trigger an AJAX save too, like you clicked a save button, thus your manual ordering will get saved too
  4. DO a CTRL-F5 or similar refresh

You see that the drag and drop ordering worked, and that the records that you manually ordered were saved

To add save button to the header of the column
After:

<?php echo JHtml::_('searchtools.sort', '', 'fp.ordering', $listDirn ...

Add:

<?php echo JHTML::_('grid.order', $this->items, 'filesave.png', 'featured.saveorder' ); ?>

Change some input manually to different order, and click on different field to blur the input,
you will see that the manually save button gets activated,
click it to save

@brianteeman
Copy link
Contributor

brianteeman commented Aug 12, 2016 via email

@ggppdk
Copy link
Contributor

ggppdk commented Aug 12, 2016

Sorry, i missed a step above:
4. After you drag and drop DO a CTRL-F5 or similar, to reload the view and force correct display of the saved fields

Maybe adding the manual save button is easier for testing (see above)

@rdeutz
Copy link
Contributor

rdeutz commented Aug 12, 2016

In fact #7368 broke it, they are two ways of fixing it. At the moment I am checking what is the best way to fix it and to have the ability to have text fields in sortable tables. PR is on the way

@rdeutz
Copy link
Contributor

rdeutz commented Aug 12, 2016

To see what is not working I used the URL-Redirection from admin-tools-pro, before the patch it doesn't work after it works. Didn't test a FOF3 component

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.

5 participants