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

Swik 1952 drag and resize boxes dissapear after pressing undo #975

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kadevgraaf
Copy link
Member

No description provided.

@abijames
Copy link
Contributor

@kadevgraaf this does improve undo and I do not lose the input boxes after using undo button or ctrl-z.
However, when I open a deck and attempt to edit a slide, input boxes do not show in slide edit. If I cancel slide edit and attempt to edit the same or a different slide then the input boxes appear. This seems to be a bug on your branch as I can't replicate on master.

Copy link
Contributor

@abijames abijames left a comment

Choose a reason for hiding this comment

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

Found bug while testing.

@abijames abijames requested a review from ali1k September 7, 2018 08:28
@ali1k
Copy link
Member

ali1k commented Sep 10, 2018

I tried to test this feature. It seems to work without removing the resize/drag boxes. However, when I repeat undo and redo several times. It gives me the following error:

Uncaught Error: Failed to execute 'setEnd' on 'Range': The offset 6 is larger than the node's length (0).
    at Dispatcher.throwOrCallErrorHandler [as _throwOrCallErrorHandler] (Dispatcher.js:170)
    at DispatcherContext.dispatch (DispatcherContext.js:117)
    at undoClick (undoClick.js:6)
    at callAction.js:21
    at t.exports (settings.js:1)
    at g.(anonymous function) (http://localhost:3000/public/js/settings.js:1:28415)
    at Number.y (settings.js:1)
    at MessagePort.b (settings.js:1)
  | throwOrCallErrorHandler | @ | Dispatcher.js:170
-- | -- | -- | --
  | dispatch | @ | DispatcherContext.js:117
  | undoClick | @ | undoClick.js:6
  | (anonymous) | @ | callAction.js:21
  | t.exports | @ | settings.js:1
  | g.(anonymous function) | @ | settings.js:1
  | y | @ | settings.js:1
  | b | @ | settings.js:1


@abijames
Copy link
Contributor

@ali1k I have tried testing this again and found it is no longer working. I cannot get input boxes on any slides. Was there any conflicts when you merged in master?

@ali1k
Copy link
Member

ali1k commented Sep 10, 2018

@abijames weird! there was no conflict with master.
Did you try napa command after you updated your branch?
CKeditor relies on some plugins that are added using napa. If you do npm i, it automatically runs napa too.

@abijames
Copy link
Contributor

@ali1k @kadevgraaf I have tested this again. I am still experiencing the problem that when I first attempt to edit a slide on a deck, there are not input boxes. I do not get this on the same decks when using master.

Also, the re-do button doesn't seem to work. The ckeditor redo button is often disabled so that you know that you cannot use it. Finally, I am not convinced about the location of the undo/redo buttons at the top of the left-menu. This gives them too much prominence over the other menu items which are all about add or changing blocks of content. There is also a problem if you scroll down to context at the bottom of the slide (necessary on smaller laptop screens) as they are no longer visible whereas the ckeditor toolbar moves to the bottom of the screen. We also have duplicate undo/redo buttons as they are still on the ckeditor toolbar. So I think this needs further work before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants