Skip to content

Conversation

@nicolasfranck
Copy link
Contributor

What it tries to fix: if you, in the org-admin, try to move the section headers, you'll get a javascript in the console.

Fix:

  • $.rails.ajax should use option type, not method
  • controller OrgAdmin::PhasesController#sort expects phase[sort_order], not just sort_order
  • concern incorrectly converted array of ids into array of arrays by prepending ids with a star.

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

looks good @nicolasfranck. Just make that one small change and we will be good 👍

method: 'post',
data: { sort_order: sectionIds },
type: 'post',
$.param({ 'phase[sort_order]': sectionIds }),
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 you're missing data: here. I receive a JS error with the above

@nicolasfranck
Copy link
Contributor Author

Fixed.

Btw, how did you manage to merge in your development branch into my branch?

See 11d9592

@briri
Copy link
Contributor

briri commented Nov 19, 2020

sorry about that! GitHub has a 'Update branch' button that shows up when the branch being PRed isn't up to date. Its a bit dangerous I guess since it updates a repo I am not a member of!

@xsrust
Copy link
Contributor

xsrust commented Nov 24, 2020

I'm additionally updating this branch, see #2741 (comment) for an explanation

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

looks good. thanks

@briri briri merged commit 05acc13 into DMPRoadmap:development Nov 25, 2020
@pherterich
Copy link

Assuming that this is about dragging template sections around, that is working for me.

portagenetwork pushed a commit to portagenetwork/roadmap that referenced this pull request Feb 24, 2022
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.

4 participants