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

Add prev/next navigation support for text, images, video. Fix undo/redo toolbox issue. #489

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

Conversation

cubiclesoft
Copy link

Unfortunately, due to a messed up master (see my other open pull request), this branch also includes the earlier code for fixing touch support for CT toolbox. (I'll do better branch-merging in the future.)

The changes here add keyboard navigation support for images and video elements to other elements. I also fixed a bug that I ran into where undo and redo via keyboard were not functioning as expected (case statements in Javascript don't accept expressions).

Partially fixes some of the issues in #487.

Related pull request: GetmeUK/ContentEdit#25

@anthonyjb
Copy link
Member

@cubiclesoft Thanks for this (and the PR on ContentEdit) I'll be merging them in this weekend (awesome work) 👍

Can I check with this PR, if I merge in this code with the touch support is there anything I should be aware of around touch support, e.g does touch support change the functionality of the editor beyond simple adding the ability to use touch events?

@cubiclesoft
Copy link
Author

#439

To get touch support working for toolbox, I reverted back to master from mobile_ready and added support for touch events in. So it's definitely a pristine commit against master. There's a demo page with just #439 if you want to test/verify:

https://cubiclesoft.com/Unrelated/ContentToolsDragTest/sandbox/

Looking back at the commit, the only obvious significant change that I see is the addition of ev.preventDefault(), which only affects touchstart (i.e. prevent browser scrolling for touch events while dragging the toolbox) whereas there is no default browser behavior for mousedown events (that I'm aware of).

@cubiclesoft
Copy link
Author

Just thought of one minor bug: When switching regions, the 'next-region' and 'previous-region' triggers will still jump to 'content' it finds instead of the navigable. That is, _handleNextRegionTransition() and _handlePreviousRegionTransition() probably need a minor adjustment or two to clean up transitioning regions if there is non-content at the start or end of a region (or if the region only contains non-content elements). This bug only affects those who use multiple regions on a page but it's no worse than the existing behavior.

@cubiclesoft
Copy link
Author

Latest commit fixes next/previous region navigation.

@anthonyjb
Copy link
Member

Thanks for the update 👍

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

2 participants