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

Make sortable groups drag sortable #1145

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from

Conversation

lipemat
Copy link
Contributor

@lipemat lipemat commented Jun 9, 2018

Fixes #155.

Builds on pull request #1142 to add global support for drag sorting groups which are sortable.

Implement using the same sortable api following the patten of cmb.makeListSortable()
Some minor CSS changes to have a move cursor.

I separated this pull out from the original because this implementation is a bit more intense and if not approved I don't want to hold up the other one.

@adriantoll
Copy link

@lipemat I've tested this on a local copy of a live site and it's working beautifully with no console errors. Nice one!

@tw2113 tw2113 requested a review from jtsternberg June 13, 2018 14:58
Copy link
Member

@jtsternberg jtsternberg left a comment

Choose a reason for hiding this comment

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

There are index issues that occur when you move things around and then try to add a new group. Will document the issues when I get a chance.

@jtsternberg
Copy link
Member

Here's a screenshot of the issues I'm seeing. http://b.ustin.co/iD9YvU I have some ideas for how to correct this, but can't get to it right now.

@adriantoll
Copy link

I can't replicate that on my local installation. I don't have something like Dropplr to share my screen, but I'm using a simpler group than @jtsternberg with just a single dropdown in each group. Replicated the exact order - start with two items (1,2), move group 2 to the top (2,1), add third group (2,1,3), save page - and it stayed as it was before saving (2,1,3).

@jtsternberg
Copy link
Member

@adriantoll you have to set the first 2 (group 1 and 2), then save, then do the move/new group, save.

@adriantoll
Copy link

@jtsternberg I didn't explain myself well - that's what I'm doing. Here's a video: https://www.dropbox.com/s/j9cw761dtndw60z/cmb2draggable.mov?dl=0

@lipemat
Copy link
Contributor Author

lipemat commented Jun 16, 2018

I took another run at this and have so far been unable to recreate the issue. Every combination of fields, sorting, adding, sorting that I have tried has saved and ordered properly.
There must be some variation that I'm not finding. Maybe a particular field type or argument?

@ljsherlock
Copy link

I was able to recreate the issue with repeatable groups.

Is this fork still being actively worked on? I would really like this functionality.

@lipemat
Copy link
Contributor Author

lipemat commented Nov 12, 2018

I spent a couple more hours trying to recreate the issue to no avail. I have also been using this drag sorting on a couple projects without any reported issues.

Perhaps, if someone could provide the configuration they are using to register the fields I could input the exact configuration to recreate?

@tw2113
Copy link
Contributor

tw2113 commented Nov 13, 2018

@lipemat lipemat force-pushed the feature/drag-sorting-for-groups branch from eaa9805 to 2a52d58 Compare November 13, 2018 11:01
@lipemat
Copy link
Contributor Author

lipemat commented Nov 13, 2018

@tw2113 Yes. Sorry about that :) I was testing against another branch and must have pushed. I have now cleaned up this feature branch.

@jtsternberg
Copy link
Member

Looks like this PR got some other commits into it (to readme, etc). Please fix merge conflicts and clean up commits/files and I'll try to reproduce the issues again.

@marsjaninzmarsa
Copy link

Fixed with #1142, so should be closed?

@lipemat
Copy link
Contributor Author

lipemat commented Mar 10, 2019

Fixed with #1142, so should be closed?

#1142 Was just for single fields. This branch ads support for groups as well.

@jtsternberg Not sure how I missed your comment :(. This branch was cleaned up last fall is ready for you to take another pass at.

@chrisvanpatten
Copy link

Really excited for this to land!

I also did some testing with a fairly complex repeatable group and was able to add, reorder, add, delete, etc. and various combinations of the above without issues. The indexes all seemed correct, even after saves and reloads.

If there's any further testing I can help with, let me know!

@chrisvanpatten
Copy link

@jtsternberg @lipemat I'd love to help push this forward. Is there anything I can do to help, either getting the branch up-to-date or formal testing or something else?

@ddzyne
Copy link

ddzyne commented Jul 19, 2019

Took the javascript modification and implemented it in the current CMB2, but it causes the TinyMCE editor inside a repeatable group not to initialize, giving you just a simple textarea instead. Any idea how to prevent this?

Edit: seems like something was up with the minified JS. Using unminified the TinyMCE works. However, after dragging and dropping a group, the TinyMCE editor of this group does break. You have to reload the page to get it working again.

…ng-for-groups

* upstream/trunk:
  Add props for CMB2#1147
  Some additional cleanup on CMB2#1147
  Add props for CMB2#1187
  Pull in the nodename, and default to div
  Make field description color accessible (fix color contrast ratio). h/t @rianrietveld. Fixes CMB2#1193
  Add optional mb_callback_args box property which allows defining args for add_meta_box. Closes CMB2#1191
  Update changelong to give @staurand props for CMB2#1190 (Fixes CMB2#1156)
  Revert file changes from CMB2#1190 and move gutenberg compatibility callback to main cmb JS file
  Update assets
  Fix tests: add cmb2-wysiwyg-gutenberg-fix as js dependency
  Fix CMB2#1156 / Update Gruntfile
  Fix CMB2#1156
  Updates 1187 - explicitly specifiy closing bracket when adding a new row to group
  support any type of markup when repeating group row
  Fix PR urls in changelog
  Add props for CMB2#1179
  Qa/phpcs cleanup (CMB2#1179)
  add our minimum required php version to the readme
  Update changelong to give props for CMB2#1177
  Another instance where we should use call_user_func() instead of direct call
  use call_user_func() instead of direct call.
  Add 3 custom field types from @scottsawyer to readme resources. Closes CMB2#1171
  update some more 3rd party resource items
  cleanup phpdocs for `php_to_js_dateformat`
  update 3rd party resources to include Leaflet Map field type. Props @villeristi
  Minor correction in URL
  Added new custom field type
  Tweak inline comments, and update changelog to give props for CMB2#1166
  Add CMB2_Field::get_rest_value method for sending value through several filters before sending to REST request
  WP_Http class constants were added in WP 4.5
  Doing it CMB2 way
  Fix the issue in WP prior to 4.7
  Add Switch Button field type to resources. Closes CMB2#1151
  make remove_default_tax_metaboxes method public so it may be called by hooks
  Call remove_default_tax_metaboxes from any type of post_hooks
  Add Props for CMB2#1142
  Update the draggable state's border style
  Fix docblock for CMB2_Type_Base::__call
lipemat added a commit to lipemat/CMB2 that referenced this pull request Jul 19, 2019
lipemat added a commit to lipemat/CMB2 that referenced this pull request Jul 19, 2019
* develop:
  Bump to 2.6.0.3
  Fix wysiwyg rendering on page reload inside repeatable groups
  [CMB2#1145] Call triggerElement when repeatable groups are drag sorted
@lipemat
Copy link
Contributor Author

lipemat commented Jul 19, 2019

Great find @daancjanssen ! Turns out there is one field in the entire plugin which requires special events to be trigger wysiwyg.

I have added these events when sorting happens as well as added the iterator to the mce ids which fixes some other strange issues.

@ddzyne
Copy link

ddzyne commented Aug 17, 2019

Great work @lipemat! Was on holiday for a few weeks, will check your mods next week.

lipemat added a commit to lipemat/CMB2 that referenced this pull request Oct 14, 2019
* feature/drag-sorting-for-groups:
  [CMB2#1145] Uglify JS
  [CMB2#1145] Update group row class when row is drag sorted or removed
  [CMB2#1145] Update group field labels and ids when row is removed
  [CMB2#1145] Reset input numbers when drag sorting group rows
  Fix wysiwyg rendering on page reload inside repeatable groups
  [CMB2#1145] Call triggerElement when repeatable groups are drag sorted
lipemat added a commit to lipemat/CMB2 that referenced this pull request Oct 14, 2019
* feature/drag-sorting-for-groups:
  [CMB2#1145] Uglify JS
  [CMB2#1145] Update group row class when row is drag sorted or removed
  [CMB2#1145] Update group field labels and ids when row is removed
  [CMB2#1145] Reset input numbers when drag sorting group rows
  Fix wysiwyg rendering on page reload inside repeatable groups
  [CMB2#1145] Call triggerElement when repeatable groups are drag sorted
lipemat added a commit to lipemat/CMB2 that referenced this pull request Oct 14, 2019
* feature/drag-sorting-for-groups:
  [CMB2#1145] Reload all wysiwyg involved in group on drag sort
Fixes issue where new rows which were then sorted did not save on first round.
Fixes JS error '$newRowId does not exist`.
lipemat added a commit to lipemat/CMB2 that referenced this pull request Dec 8, 2019
Fixes issue where new rows which were then sorted did not save on first round.
Fixes JS error '$newRowId does not exist`.
@Pierronimo
Copy link

Hi everybody.
I'm very interested in this feature and look like you already did a great job !
Can I help in anything ?

@tw2113
Copy link
Contributor

tw2113 commented Sep 8, 2020

Could probably use some sort of testing of the new changes to make sure we're no potentially breaking things, as a good next step.

@Pierronimo
Copy link

Hi everybody,

I did some test about the issue @jtsternberg report and could repeat it in a more simple way :

  • add group 2
  • move group 1 to 2nd position
  • save
    => only group in first position is saved

Looking in the code of the page, the id of the group in 2nd position when saved (the one that is moved) still have the original id before the drag and drop.
If you change manually this ID to its real position after drag and drop, everything is saved correctly.

Screenshot before drag looking at ID and name of the "D1"
before drag

Screenshot after drag looking at ID and name of the "D1 => D2", now in position 2
after drag

At this point, if you change manually the ID and Name, everything is ok when you save

You can see a video of the step I follow here : https://web.pierreferron.com/hosting/cmb2-issue-720p.mov

I'm not very good at coding, especially javascript, I can give a try if you can indicate me exactly where the ID and Name are updated in the code

I will do more test with more groups and movement.

For the TinyMCE issue, it is not just with dragable CMB2 group, if you drag an entire block of the wordpress page, you have the same issue, the content deseappear, but it is saved in DB.

Hope this help and sorry for my post, I'm new with github and coding

@greenlevel
Copy link

Just checking the progress of the feature.

Is this in the planning to be added anytime soon as this would be nice to have.

Thanks!

@tw2113
Copy link
Contributor

tw2113 commented Oct 5, 2022

No news that I've heard of. We tend to do pretty well with leaving development updates on the issues themselves.

@greenlevel
Copy link

Thanks for the feedback. Was hoping this feature would be already or soon be implemented.
But I understand.

lipemat added a commit to lipemat/CMB2 that referenced this pull request Jan 27, 2023
A new version of the original fix done in this pull request
CMB2#1145
@rodiuk
Copy link

rodiuk commented Mar 31, 2023

https://wordpress.org/support/topic/editing-repeateble-field-delete-text-value/#post-16602143

(https://youtu.be/1eGm3Po_k74)
i commented old CMB2 plugin and download from GIThub

bug still stayed

Please check if the WYSIWYG field works correctly in the repeater. Because I tested according to the instructions, and the problem is only with the WYSIWYG field. You can see the whole process in the video from the previous post.

@tw2113
Copy link
Contributor

tw2113 commented Mar 31, 2023

@rodiuk are you saying that the issue still exists in the lipemat:feature/drag-sorting-for-groups branch that's open for this PR? Or that it's just still open in CMB2 trunk?

@rodiuk
Copy link

rodiuk commented Apr 1, 2023

@rodiuk are you saying that the issue still exists in the lipemat:feature/drag-sorting-for-groups branch that's open for this PR? Or that it's just still open in CMB2 trunk?

I don't know what thread should be.
I described my problem here https://wordpress.org/support/topic/editing-repeateble-field-delete-text-value/#post-16602143

but received no reply.

You can create a new branch if that's the right way.

Due to the incorrect work of the plugin, I have a lot of inconvenience and need to find a solution for my client. I beg you to check my video and let me know whether to wait for the update.

@tw2113
Copy link
Contributor

tw2113 commented Apr 1, 2023

I don't have or know of a solution for the issue at the moment. You're commenting on someone else's PR, which is perfectly fine to do, but I am wondering if you tried this person's PR out and if the PR is not yet fixing the issue.

You didn't comment on an open GitHub issue about the bug at hand.

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.