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

External link includes selected text #4366

Merged
merged 10 commits into from Mar 20, 2018

Conversation

Projects
None yet
6 participants
@tonyyates
Copy link
Contributor

tonyyates commented Mar 14, 2018

As described in issue #4328 selected text wasn't populated when adding
a external link. This commit fixes that issue by borrowing from the following functions :

getSelectedBlocksMap()
getSelectedBlocksList()
getSelectedText()
from https://github.com/jpuri/draftjs-utils/blob/e81c0ae19c3b0fdef7e0c1b70d924398956be126/js/block.js#L106

I am not sure new tests are needed?
Tested on Firefox, Chrome, Opera and Safari

New behaviour:

kapture 2018-03-14 at 16 12 47

@@ -35,7 +84,7 @@ export const getChooserConfig = (entityType, entity) => {
// This does not initialise the modal with the currently selected text.
// This will need to be implemented in the future.

This comment has been minimized.

@gasman

gasman Mar 14, 2018

Collaborator

Please can you remove this comment, now that it's no longer true? :-)

This comment has been minimized.

@tonyyates

tonyyates Mar 14, 2018

Contributor

hmm I did start the commit, realised this and ganged the file before saving, I thought I had done this, will sort though. Thanks for reviewing 👍

@AlexsMathilda

This comment has been minimized.

Copy link

AlexsMathilda commented Mar 14, 2018

I've tested the external link includes selected text on Windows IE11, MS Edge, latest Chrome and Firefox. MacOS 10.13 Safari, latest Chrome and Firefox Android Chrome 7, iOS Safari 10.

the issue is fixed

@thibaudcolas thibaudcolas self-requested a review Mar 14, 2018

@thibaudcolas thibaudcolas added this to the 2.0.1 milestone Mar 14, 2018

@thibaudcolas

This comment has been minimized.

Copy link
Member

thibaudcolas commented Mar 14, 2018

👏 I'll have a look at this hopefully before the end of the sprint, otherwise later on, and take care of the tests at the same time.

Thank you for the manual testing @AlexsMathilda!

@tonyyates

This comment has been minimized.

Copy link
Contributor

tonyyates commented Mar 14, 2018

@thibaudcolas I am happy to write the tests tomorrow if you can give me a few pointers, I am not sure how the client tests currently work but I am a keen student.

@thibaudcolas

This comment has been minimized.

Copy link
Member

thibaudcolas commented Mar 14, 2018

@tonyyates cool, let's do that tomorrow then 🙂

@gasman

gasman approved these changes Mar 15, 2018

Copy link
Collaborator

gasman left a comment

All working very nicely here! Particularly happy to see the fiddly prefer_this_title_as_link_text logic from #3039 still doing the right thing on Draftail (namely, not clobbering any inline formatting when the link text is left unchanged) :-)

@tonyyates

This comment has been minimized.

Copy link
Contributor

tonyyates commented Mar 15, 2018

@gasman with thanks to @thibaudcolas for his help showing me how the tests work with jest. I have now included a number of tests that make this more robust.

screenshot 2018-03-15 16 22 50

@thibaudcolas

This comment has been minimized.

Copy link
Member

thibaudcolas commented Mar 18, 2018

👏 sorry I didn't get to this during the sprint, I'll review + merge the PR this week.

@thibaudcolas thibaudcolas force-pushed the vixdigital:4328-external-link-selected-text branch from 6a0de1c to cd201dd Mar 19, 2018

@thibaudcolas
Copy link
Member

thibaudcolas left a comment

This seems to be working well! I did some refactorings, still have more testing to do but this looks good code-wise.

This isn't strictly related to this PR but I find this feature quite confusing when adding a link over multiple blocks at once:

link-text-override

I don't see a way to make this work though (single-line field will never fit a multi-line selection). Again, this isn't related to those changes so not worth holding this PR for that.

const blockStart = i === 0 ? start : 0;
const blockEnd = i === (selectedBlocks.size - 1) ? end : selectedBlocks.get(i).getText().length;
selectedText += selectedBlocks.get(i).getText().slice(blockStart, blockEnd);
}

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 19, 2018

Member

I removed a if selectedBlocks.size > 0 conditional for this logic – there doesn't seem to be any cases where there is 0 selected blocks.

selection = selection.merge({
anchorOffset: 8,
focusOffset: 4,
isBackward: true,

This comment has been minimized.

@thibaudcolas
import { EditorState, convertFromRaw, AtomicBlockUtils, RichUtils, Modifier } from 'draft-js';

global.ModalWorkflow = () => {};

describe('ModalWorkflowSource', () => {
beforeEach(() => {
jest.spyOn(global, 'ModalWorkflow');
jest.spyOn(DraftUtils, 'getSelectionText').mockImplementation(() => '');

This comment has been minimized.

@thibaudcolas

thibaudcolas Mar 19, 2018

Member

Added mocking for this for the related test suites.

@thibaudcolas
Copy link
Member

thibaudcolas left a comment

👍 tested in latest Chrome, Firefox, Safari on macOS 10.13, IE11 & latest Edge in Windows, iOS 11 Safari.

Merging

tonyyates and others added some commits Mar 14, 2018

External link includes selected text
As described in issue #4328 selected text wasn't populated when adding
a external link. This commit fixes that issue.
Refactored and included jest tests
Refactored the code to be simpler brought in from drafttail.
Added tests for these functions that test the ability to handle empty
selections, backwards selections and multi block content.

@thibaudcolas thibaudcolas force-pushed the vixdigital:4328-external-link-selected-text branch from 5e16e20 to e507614 Mar 20, 2018

@thibaudcolas thibaudcolas merged commit cab90e5 into wagtail:master Mar 20, 2018

0 of 2 checks passed

ci/circleci Your CircleCI tests were canceled
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@tomdyson

This comment has been minimized.

Copy link
Contributor

tomdyson commented Mar 20, 2018

Thanks @tonyyates and @thibaudcolas!

@tonyyates

This comment has been minimized.

Copy link
Contributor

tonyyates commented Mar 20, 2018

BertrandBordage added a commit to BertrandBordage/wagtail that referenced this pull request Apr 16, 2018

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