Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes #855 Update Slate plugins to drop data arg #856

Merged
merged 7 commits into from
Dec 14, 2017

Conversation

Dammmien
Copy link
Contributor

@Dammmien Dammmien commented Nov 30, 2017

- Summary

Fixes #855 Update Slate plugins to drop data arg

- Test plan

  • Check SoftBreak and BreakToDefaultBlock are okay.
  • Check you can use cmd + z and cmd + y to undo and redo.
  • Check you can toggle marks with cmd + ( b, i, s, u ).

- Description for the changelog

  • Remove data arg due to breaking changes introduced in 0.6.0.
  • Use is-hotkey.
  • Fix underline typo for togglemark cmd + u ( but doesn't work either it throws an error ).
  • Check next is defined before changeHistory to avoid error length of undefined.

@@ -20,10 +21,9 @@ function changeHistory(change, type) {
*/
const next = historyOfType.first();
const historyOfTypeIsValid = historyOfType.size > 1
|| next.length > 1
|| next[0].type !== 'set_selection';
|| ( next && next.length > 1 && next[0].type !== 'set_selection' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the logic - should evaluate true if next.length > 1 OR next[0].type !== 'set_selection'. I also prefer multiline for readability. If there was no bug here, let's just remove the changes and leave as is.

Copy link
Contributor

@erquhart erquhart Dec 6, 2017

Choose a reason for hiding this comment

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

Oh I see, you're pulling the next check up - you can just prepend it:

const historyOfTypeIsValid = next && historyOfType.size > 1
(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that because there was a bug, sometimes next is undefined.
But I don't understand this logic : next.length > 1 OR next[0].type.
It will throw an error if next.length === 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually pulled this logic from Slate's core - we could make it safe by changing the last line to: || (next[0] && next[0].type !== 'set_selection');

@verythorough
Copy link
Contributor

Deploy preview ready!

Built with commit 3a76293

https://deploy-preview-856--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 8, 2017

Deploy preview ready!

Built with commit 6901088

https://deploy-preview-856--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 14, 2017

Deploy preview ready!

Built with commit 6901088

https://deploy-preview-856--cms-demo.netlify.com

@erquhart erquhart force-pushed the 855_update_slate_plugin branch from b9c7203 to 6901088 Compare December 14, 2017 22:59
@erquhart erquhart merged commit 78ad082 into decaporg:master Dec 14, 2017
@Dammmien Dammmien deleted the 855_update_slate_plugin branch April 26, 2018 20:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants