Re list bug fix #195

robinedman · 2014-06-12T13:15:36Z

Fixes oja-list-bug #184

Also changes the amd.html example not to use custom elements, as this caused issues when testing this (NSERROR when doing insertOrderedList with the caret at the end of a 1 line paragraph).

…ing BR elements

…refox

…ld work)

robinedman · 2014-06-12T14:39:31Z

I agree that some helpers would be nice (that native API is interesting to put it nicely :)). I like the idea of immutable ones as well . Is there no lightweight library doing this though? (Google mostly points me towards React...)

OliverJAsh · 2014-06-12T14:46:05Z

@robinedman I had a look around before, it doesn't look like there is anything.

OliverJAsh · 2014-06-12T14:46:50Z

src/plugins/core/patches/commands/insert-list.js

                listParentNode.parentNode.insertBefore(listElement, listParentNode.nextElementSibling);
                selection.selectMarkers();
-                listParentNode.parentNode.removeChild(listParentNode);
+                if (listParentNode.childNodes.length === 0) {


A comment to say why we do this would be good (because sometimes the paragraph still contains content).

OliverJAsh · 2014-06-12T14:47:17Z

Nice fix! 👍

OliverJAsh · 2014-06-12T14:47:39Z

(We’ll wait for the build to finish before merging. See you next year.)

robinedman · 2014-06-12T14:48:44Z

Some libraries here: http://microjs.com/#dom

They generally (maybe always) include things we don't care about though. Possibly if there was one that was good enough, not too weird (thinking of others who want to contribute to Scribe), and that could be custom built with the helpers we want.

OliverJAsh · 2014-06-12T14:55:19Z

I don't want the monad-like(?) API that libraries such as jQuery provide –
I would prefer just to have functions, i.e. insertAfter(element, afterElement) instead of someWrapper(element).insertAfter(afterElement),
because it’s simpler to start with.

On the other hand, I think front-end uses Zepto for their DOM manipulation.
If we do go with a library, we should consider how this is brought in. A
good example of doing this right is how we bring in Lodash – we use the
lodash-amd project, which lets us require individual functions. I wouldn't
want to bundle in loads of library code we never use.

Oliver Joseph Ash
Software Engineer

On 12 June 2014 15:48, Robin Edman [email protected] wrote:

Some libraries here: http://microjs.com/#dom

They generally (maybe always) include things we don't care about though.
Possibly if there was one that was good enough, not too weird (thinking of
others who want to contribute to Scribe), and that could be custom built
with the helpers we want.

—
Reply to this email directly or view it on GitHub
#195 (comment).

Visit theguardian.com. On your mobile and tablet, download the Guardian
iPhone and Android apps theguardian.com/guardianapp and our tablet editions
theguardian.com/editions. Save up to 57% by subscribing to the Guardian
and Observer - choose the papers you want and get full digital access.
Visit subscribe.theguardian.com

This e-mail and all attachments are confidential and may also be
privileged. If you are not the named recipient, please notify the sender
and delete the e-mail and all attachments immediately. Do not disclose the
contents to another person. You may not use the information for any
purpose, or store, or copy, it in any way. Guardian News & Media Limited
is not liable for any computer viruses or other material transmitted with
or as part of this e-mail. You should employ virus checking software.

Guardian News & Media Limited is a member of Guardian Media Group plc. Registered
Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered
in England Number 908396

OliverJAsh · 2014-06-12T15:10:43Z

examples/amd.html

-</rte-toolbar>
-<rte></rte>
+<div class="rte-toolbar">
+  <button data-command-name="insertOrderedList">Ol</button>


Did you mean to change this?

OliverJAsh · 2014-06-12T15:44:05Z

Re. immutable helpers, @domenic thinks there could be performance issues: https://twitter.com/domenic/status/477113074235613184. I’m not sure how we can really measure this.

OliverJAsh · 2014-06-12T16:19:26Z

The build is still running, but the earliest results demonstrate a small problem in Firefox: https://travis-ci.org/guardian/scribe/jobs/27416128

Note: we didn't have a green build to begin with – there is a known failing test on Firefox 23/24/25. We should get around to solving that (separately from this).

OliverJAsh · 2014-06-16T16:05:24Z

src/plugins/core/patches/commands/insert-list.js

+
+            if (listElement.nextElementSibling &&
+                listElement.nextElementSibling.textContent === '') {
+              listElement.nextElementSibling.parentNode.removeChild(listElement.nextElementSibling);


The next sibling will share the same parent, no?

OliverJAsh · 2014-06-17T10:27:01Z

src/plugins/core/patches/commands/insert-list.js

+            /**
+             * Chrome: If we apply the insertOrderedList or the insertUnorderedList
+             * command on an empty block, the OL/UL will be nested inside the block.
+             * As per: http://jsbin.com/oDOriyU/1/edit?html,js,output


Not your mistake, but I think the isolated case here should be http://jsbin.com/eFiRedUc/1/edit. Or am I going crazy?

OliverJAsh · 2014-06-17T15:51:45Z

src/plugins/core/patches/commands/insert-list.js

+                /**
+                 * Chrome 27-34: An empty text node is inserted.
+                 */
+                if (listParentNode.childNodes.length === 2 &&


We probably don't need to check for childNodes.length === 2 but it does no harm.

OliverJAsh · 2014-06-17T15:52:30Z

This looks good to me! Nice work 😄

OliverJAsh · 2014-06-17T15:52:33Z

👍

theefer · 2014-06-17T15:55:26Z

👍

TooTallNate · 2014-06-17T16:00:01Z

bower.json

@@ -1,7 +1,8 @@
 {
  "name": "scribe",
  "dependencies": {
-    "lodash-amd": "2.4.1"
+    "lodash-amd": "2.4.1",
+    "scribe-common": "0.0.4"


Please add this entry to the package.json file as well.

Thanks for pointing that out @TooTallNate. I've added it now.

Re list bug fix

Oliver Joseph Ash and others added 6 commits June 2, 2014 14:15

Add failing test

7bc88e4

Merge branch 'master' into oja-list-bug

c44ebe9

Fixes #184 - Text is lost when creating a list from P element contain…

fb31fa9

…ing BR elements

Changed example not to use custom elements as they cause issues in Fi…

f340766

…refox

Changed back to nextElementSibling instead of nextSibling (either wou…

c1122b2

…ld work)

Removed redundant comments

227c96f

OliverJAsh reviewed Jun 12, 2014
View reviewed changes

Clarifying comment

d640bf0

OliverJAsh reviewed Jun 12, 2014
View reviewed changes

Removes accidentally commited debugging code

ea7f0a5

Documented and fixed Firefox issue and improved comments

945abe7

OliverJAsh reviewed Jun 16, 2014
View reviewed changes

robinedman added 2 commits June 16, 2014 17:28

Improved conditions

e7e42c1

Fix for Chrome 27 to 34

dc86bf5

OliverJAsh reviewed Jun 17, 2014
View reviewed changes

robinedman added 2 commits June 17, 2014 16:35

Use new scribe-common helpers

32dcb69

Fixed link to wrong isolated case

63e3ea4

OliverJAsh reviewed Jun 17, 2014
View reviewed changes

TooTallNate reviewed Jun 17, 2014
View reviewed changes

Added scribe-common to package.json

a8195fa

TooTallNate mentioned this pull request Jun 17, 2014

Fix npm/browserify usage guardian/scribe-common#2

Merged

OliverJAsh pushed a commit that referenced this pull request Jun 17, 2014

Merge pull request #195 from guardian/re-list-bug-fix

1fb48f9

Re list bug fix

OliverJAsh merged commit 1fb48f9 into master Jun 17, 2014

OliverJAsh deleted the re-list-bug-fix branch June 17, 2014 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re list bug fix #195

Re list bug fix #195

robinedman commented Jun 12, 2014

robinedman commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

robinedman commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 16, 2014

OliverJAsh Jun 17, 2014

OliverJAsh Jun 17, 2014

OliverJAsh commented Jun 17, 2014

OliverJAsh commented Jun 17, 2014

theefer commented Jun 17, 2014

TooTallNate Jun 17, 2014

robinedman Jun 17, 2014

Re list bug fix #195

Re list bug fix #195

Conversation

robinedman commented Jun 12, 2014

robinedman commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 12, 2014

Choose a reason for hiding this comment

OliverJAsh commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

robinedman commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 12, 2014

Choose a reason for hiding this comment

OliverJAsh commented Jun 12, 2014

OliverJAsh commented Jun 12, 2014

OliverJAsh Jun 16, 2014

Choose a reason for hiding this comment

OliverJAsh Jun 17, 2014

Choose a reason for hiding this comment

OliverJAsh Jun 17, 2014

Choose a reason for hiding this comment

OliverJAsh commented Jun 17, 2014

OliverJAsh commented Jun 17, 2014

theefer commented Jun 17, 2014

TooTallNate Jun 17, 2014

Choose a reason for hiding this comment

robinedman Jun 17, 2014

Choose a reason for hiding this comment