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

Fix list item reordering #437 #493

Merged
merged 2 commits into from Jul 25, 2017

Conversation

Dammmien
Copy link
Contributor

- Summary
Fix list item reordering. As explain in the issue #437.

- Test plan
In the example project:

  • Open the authors entry in the settings collection
  • Update the author descriptions to be distinct
    ( editor have to be on visual mode, markdown disabled )
  • Reorder the authors
  • Check the descriptions
  • Before, the descriptions wasn't correctly updated, now it should be correct.

- Description for the changelog
After component did update, check the editor content is the same that the state value.
If it isn't the same, reset the state using a mutualized createState method.
I think it isn't the cleanest way to fix this bug.
It's better than nothing but if you have any other idea ...

@erquhart
Copy link
Contributor

Thanks for this!

I think it isn't the cleanest way to fix this bug.
It's better than nothing

My thoughts exactly. The visual editor is coming to the end of a rewrite, so I have no problem merging this for now.

@erquhart erquhart requested a review from Benaiah July 24, 2017 17:10
Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

Given that this is a temporary fix, this LGTM. My only suggestion might be to add a comment in the code explaining why this is being done, so it's not refactored away by a well-meaning contributor. That's a low risk, though, so IMO it's fine either way.

@erquhart
Copy link
Contributor

That's a good point - @Dammmien would you mind adding a comment in the new lifecycle method explaining it's purpose? Then we're good to merge.

@Dammmien
Copy link
Contributor Author

Done @erquhart !

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Thanks!

@erquhart erquhart merged commit 56e63b6 into decaporg:master Jul 25, 2017
@Dammmien Dammmien deleted the fix_list_item_reordering branch April 26, 2018 20:15
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

3 participants