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

De-listing #540

Merged
merged 21 commits into from Jun 30, 2014

Conversation

Projects
None yet
2 participants
@mightyiam
Member

mightyiam commented Jun 19, 2014

Not for merging yet.

@mightyiam mightyiam referenced this pull request Jun 19, 2014

Merged

De-listing #320

@mightyiam

View changes

src/test/unit/specific_feature_tests/lists.js Outdated
@@ -2388,19 +2405,19 @@ test("Ordered to unordered one item", function () {
testListRoundTrip('li_1_1_1_1', 'ordered', li_1_1_1_1_unorderedHtml, orderedHtml, true);
});
test("Prevent converting type with selection over multiple levels", function () {
test("Converting type with selection over multiple levels", function () {

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

Why not?

// This loop is required for IE7 because it seems to populate
// the attributes property with all possible attributes. When
// support for IE7 is dropped the length check should be
// enough.

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

Some IE7 love.

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

If there's a problem with this code than it could result in some useless span elements after de-listing.

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

And this should be only in IE7. 🐘

this.setCaretIn(newNode);
if (setCaret) {
wym.setCaretIn(newElement);
}

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

The default behavior was to set caret. But I don't understand why. At least, none of our own code ever uses it.

It doesn't seem useful to me. So, I can guess that no third party plugins use it. But can we be sure?

Will it be ok to remove it? Or should I put out a deprecation warning to it for removal in 1.0.0?

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

At least, I think that it is useless. I'm not sure. Tests pass. But is this covered? Really, why would it even necessary to perform this action? I'm trying to understand.

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

It's probably not covered in tests, but I'm on board with changing the default. This should be documented under backwards incompatible changes, though.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

Documented. Thanks.

@mightyiam

View changes

src/wymeditor/editor/base.js Outdated
keepSublistContentsIndentation =
typeof keepSublistContentsIndentation !== 'undefined' ?
keepSublistContentsIndentation : true;

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

This—I don't know where this originated. It isn't mine. I've started this work based on some branch but I'm not sure which.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

I still don't know what this is. Will check.

@mightyiam

View changes

src/wymeditor/editor/base.js Outdated
$sublistContents.length > 0 &&
// This is a trigger to prevent this
keepSublistContentsIndentation === true
) {

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

And this—not mine.

@mightyiam

View changes

src/wymeditor/editor/base.js Outdated
If the selection is already inside a list, switch the type of the nearest
parent list to an `<ol>`. If the selection is in a block element that can be a
valid list, place that block element's contents inside an ordered list.
See in-line comments for an understanding.

This comment has been minimized.

@mightyiam

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

Negative. Docstrings are for people that need to use the function. Inline comments are good, but they aren't a substitute. Inline comments can explain the why, but the docstring needs to explain to a potential user what this function does.

// change the type of that list.
if (listItems.length !== 0) {
// If the selection is across paragraphs and other items at the root level,
// don't indent

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

Why not?

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

Because I hadn't throught through all of the corner cases. Have you?

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

Generally, I would prefer to push this functionality change to another issue. Otherwise, it could potentially delay #320.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

Thanks. Done.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

You can see it reimplemented below.

// We have a selection across multiple root-level lists. Punt on
// this case for now.
// TODO: Handle multiple root-level lists properly
return false;

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

I guess I would have to change it back to "punt".

This comment has been minimized.

@mightyiam
// convert it.
selectedBlock = this.selectedContainer();
// Get a potential block element from selection that could be converted
// into a list:
// TODO: Use `_containerRules['root']` minus the ol/ul and
// `_containerRules['contentsCanConvertToList']

This comment has been minimized.

@mightyiam

mightyiam Jun 19, 2014

Member

What does this mean? Should I implement?

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 19, 2014

Regarding restoreSelectionAfterManipulation, it uses rangy to ensure that the user's selection isn't destroyed by DOM manipulation. If you destroy and recreate a node in the beginning of their selection, for instance, without that, their selection can go poof.

@mightyiam

This comment has been minimized.

Member

mightyiam commented Jun 19, 2014

@winhamwr

Regarding restoreSelectionAfterManipulation, it uses rangy to ensure that the user's selection isn't destroyed by DOM manipulation. If you destroy and recreate a node in the beginning of their selection, for instance, without that, their selection can go poof.

What do you mean by "in the beginning of their selection", please?

@mightyiam

This comment has been minimized.

Member

mightyiam commented Jun 19, 2014

I've done some manual testing and have never seen a selection evaporating.

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 19, 2014

I've done some manual testing and have never seen a selection evaporating.

Cool. Sounds like it's not a problem for de-listing, then.

@winhamwr

View changes

src/wymeditor/editor/base.js Outdated
@param node The node.
*/
WYMeditor.editor.prototype.hasAttributes = function (element) {

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

What do you mean by valuable attributes? Valuable to what? The name hasAttributes implies that this checks whether or not an element has any attributes. Needs a better docstring and name.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

Thanks Done.

@winhamwr

View changes

src/wymeditor/editor/base.js Outdated
@param listItems A jQuery object of list items.
*/
WYMeditor.editor.prototype._removeItemsFromList = function ($listItems) {

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

No need for blank line.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

Oops. Thanks. I'll fix my jshint.

@winhamwr

View changes

src/wymeditor/editor/base.js Outdated
// TODO: what about this? Could this be a better behavior?
// It is reasonable to de-list only a subset of the provided list items.
// The subset are the list items which are not ancestors of any other list

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

You could have a list in a td in a table that wouldn't be an ancestor of any other list item, but we wouldn't want to delist that just because we were trying to de-list the root.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

Thanks. Done.

@winhamwr

View changes

src/wymeditor/editor/base.js Outdated
// Determine the type of element this list item will be transformed
// into and call for this transformation.
if ($listItem.parent().parent('li').length === 1) {
// The list item will end up inside another list item. Turn it into

This comment has been minimized.

@winhamwr

winhamwr Jun 20, 2014

Member

We don't want to create empty spans, right?

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 20, 2014

Does this branch pass with all of the cases you showed in the video a while back?

@mightyiam mightyiam added bug labels Jun 23, 2014

@mightyiam mightyiam added this to the 1.0.0b7 milestone Jun 23, 2014

@mightyiam mightyiam self-assigned this Jun 23, 2014

delistHtml.withTable_parentDeListed,
true
);
});

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

De-listing the parent of a table.

// the inclusion of the list item that might be an ancestor of the
// table.
return [];
}

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

This should be pretty clear with this comment.

if ($selectedNodes.closest('li, table').filter('li').length === 0) {
// Selection is in a table before it is in a list. This prevents
// inclusion of the list item that the table may be contained
// in.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

This should also be quite clear.

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

It is a similar concept to the above case with the collapsed selection.

Returns `true` if a change was made, `false` otherwise.
@param listType A string, representing the user's action, either 'ul'
or 'ol'.
*/

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

I hope this docstring is awesome.

}
};
/**

This comment has been minimized.

@mightyiam

mightyiam Jun 23, 2014

Member

This is my biggest function to date.

This comment has been minimized.

@winhamwr

winhamwr Jun 23, 2014

Member

I'll have to give this a thorough look tomorrow.

@mightyiam

This comment has been minimized.

Member

mightyiam commented Jun 23, 2014

Not ready for merge.

@mightyiam

This comment has been minimized.

Member

mightyiam commented Jun 24, 2014

Does this branch pass with all of the cases you showed in the video a while back?

All of them, except the last one, shown at 8:45. Video.

How important do you consider it?

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 25, 2014

How important do you consider it?

What happens instead?

@winhamwr

View changes

src/wymeditor/editor/base.js Outdated
// https://github.com/wymeditor/wymeditor/issues/541
commonParentList = wym.getCommonParentList(listItems, true);
if (commonParentList) {
wym._changeListType(commonParentList, listType);
return true;

This comment has been minimized.

@winhamwr

winhamwr Jun 25, 2014

Member

Looks like indentation is off here.

This comment has been minimized.

@mightyiam

mightyiam Jun 26, 2014

Member

Thanks. Done.

}
// `br`s may have been transferred to the root container. They don't
// belong there.
jQuery(wym._doc).find('body.wym_iframe').children('br').remove();

This comment has been minimized.

@winhamwr

winhamwr Jun 25, 2014

Member

We insert spacer brs between consecutive images/tables so that they're editable by users. You can see tests for this feature inside blocking_elements.js. This is done by the spaceBlockingElements() call inside fixBodyHtml. I think there's currently a bug here where those br's will be removed and it won't be possible to put content between consecutive tables in the root of the document.

If you create test data with two consecutive tables at the root, you should be able to see the bug (although I can't remember in which browsers it appears). I think just calling fixBodyHtml as the last thing in this function should fix things.

// no use for it being a span.
attributes = $listItem[0].attributes;
if (!wym.hasRealAttributes($listItem[0])) {
$listItem.before($listItem.contents());

This comment has been minimized.

@winhamwr

winhamwr Jun 25, 2014

Member

The wym.unwrap helper method should do this, right?

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 25, 2014

This is looking really good. I especially appreciated the careful work with span elements to hold a space. The fix for the blocking_elements thing should be easy and my other comments were minor. If you feel you understand and can address all of these, I'm +1 on you merging this in at your convenience.

As far as the single case from the video that you don't handle, as long as it doesn't throw an error, re-order/lose content, then I'm totally cool with merging this in anyway.

Very Excite

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 30, 2014

@mightyiam Do you not have jshint running in your editor? I've gotten jshint problems in all of the files I've touched so far.

@winhamwr

This comment has been minimized.

Member

winhamwr commented Jun 30, 2014

@mightyiam We should talk about on which repo you push your work. It would have been really nice for me to be able to edit your branch directly, versus needing to create another branch. Is there a reason you aren't working in wymeditor/issue_540 instead of mightyiam/issue_540?

@winhamwr winhamwr merged commit b975edb into wymeditor:master Jun 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@mightyiam mightyiam deleted the mightyiam:issue_320_lap_2 branch Jul 2, 2014

@mightyiam

This comment has been minimized.

Member

mightyiam commented Jul 2, 2014

@winhamwr I'll start using the official repo.

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