New: Footer transforms #53

montehurd · 2017-07-12T23:54:07Z

https://phabricator.wikimedia.org/T164137

As these are consolidated I'm concurrently updating this iOS PR.

New transforms:

container
menu
read more
legal

Misc:

compatibility for older devices (audit array methods etc)

Needed follow-ups:

expand test coverage
add demos
consider further internals refactors/simplifications.

Feedback comments which I've yet to address or which we haven't come to consensus on yet:
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)
#53 (review)

montehurd · 2017-07-13T01:27:16Z

Wondering if we should enable the valid-jsdoc eslint rule...

niedzielski · 2017-07-13T02:25:06Z

@montehurd, yes! Please enable valid-jsdoc. I'm sorry but I thought we already had it enabled and was mistaken.

niedzielski · 2017-07-13T02:26:00Z

src/transform/FooterContainer.css

@@ -0,0 +1,25 @@
+
+.footer_container {


It's fine to punt on this but consider using a common prefix like pagelib-footer-thing.

Good idea! Once I get Read more into this PR I'll make a wholesale change.

Fixed! (I didn't change from underscores to dashes yet though)

niedzielski · 2017-07-13T02:28:17Z

src/transform/FooterContainer.js

+  const elements = document.querySelectorAll(
+    '#footer_container_menu_heading, #footer_container_readmore, #footer_container_legal'
+  )
+  Array.from(elements)


I see you already called this out in the todo but consider replacing Array.from() with Array.prototype.slice.call() for old time Android devices.

Fixed with polyfill.

niedzielski · 2017-07-13T02:34:09Z

src/transform/FooterMenu.css

+}
+
+.footer_menu_icon_disambiguation {
+    background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAFAAAABQCAYAAACOEfKtAAAAAXNSR0IArs4c6QAAAWRJREFUeAHt1ruOwjAQBVAW8YnbbMHXUdDwj6xWKyigGJLrcaTo0PBwBifH1xofDl4ECBAgQIAAAQIECBAgQIAAAQIECBAgQGCGwNeMSf7m+P4532fNtWSe2/USGRyXTObad4HT+0+9v6QrPuruRu0ICQxXBCDAUCAsl0CAoUBYPqwLf9rVPr0ufK5neXfXt4Wf1Os+DEvgY/ruFX/MU73PSroEVitRjAMsgKphgJVQMQ6wAKqGAVZCxTjAAqgaBlgJFePDz4Gv8806j211/pTA1xVf+L09gVslY6HD6sslcDXdfyFAgKFAWC6BAEOBsLy9CzsHhiu09/L2BDoH7j1C4fPpwgBDgbBcAgGGAmF5exd2DgxXaO/l7Qnc+hzYvQM0kXCLtCcwvL+4vHsHSGC4RAABhgJhuQQCDAXC8uFduPvcFT7v8HJbeDipPyRAgAABAgQIECBAgAABAgQIECBAYK8Cv3qKKmmwPBETAAAAAElFTkSuQmCC);


I wonder why GitHub is coloring this line differently.

Ha I was wondering about this too!

niedzielski · 2017-07-13T02:35:06Z

src/transform/FooterMenu.js

+ * Extracts array of no-html page issues strings from document.
+ * @type {FooterMenuItemPayloadExtractor}
+ */
+const pageIssuesStringsArray = document => {


This seems to be a method but uses @type.

See the FooterMenuItemPayloadExtractor typedef for the details. I think I did this correctly according to the Type definitions box in the docs. Lemme know if not :)

👍 this looks good! My mistake!

niedzielski · 2017-07-13T02:35:58Z

src/transform/FooterMenu.js

+  payloadExtractor() {
+    switch (this.itemType) {
+    case MenuItemType.languages:
+      return null


Please use undefined instead of null per earlier discussion.

niedzielski · 2017-07-13T02:38:01Z

src/transform/FooterMenu.js

+      item.classList.add(iconClass)
+    }
+
+    return document.createDocumentFragment().appendChild(item)


Is it intentional to a return a value from the constructor?

Fixed! (Fragment makers can just be functions.)

niedzielski · 2017-07-13T02:38:35Z

src/transform/index.js

@@ -9,6 +12,9 @@ import WidenImage from './WidenImage'

 export default {
  CollapseTable,
+  FooterContainer,


This is great how you have these broken out!

montehurd · 2017-07-13T04:01:16Z

src/transform/FooterLegal.css

+  background-position: right top;
+}
+
+.footer_legal_licence {


…ids.

niedzielski · 2017-07-14T14:40:01Z

src/transform/FooterContainer.css

+}
+
+#pagelib_footer_container_readmore_pages::-webkit-scrollbar {
+  display: none;


I'm sure you tried this already but does overflow: hidden not work?

I removed this - was unused.

niedzielski · 2017-07-14T14:40:53Z

src/transform/FooterContainer.js

+ * @return {void}
+ */
+const updateBottomPaddingToAllowReadMoreToScrollToTop = (document, window) => {
+  const div = document.getElementById('pagelib_footer_container_ensure_can_scroll_to_top')


Consider making a symbol for this ID.

Ok to hold off on this for now? The template complicates doing this... it's filled with such ids...

niedzielski · 2017-07-14T14:44:41Z

src/transform/FooterContainer.js

+
+/**
+ * Ensures the 'Read more' section header can always be scrolled to the top of the screen.
+ * @param {!Document} document


Do we want to have a document parameter or can we just use window.document?

niedzielski · 2017-07-14T14:49:16Z

src/transform/FooterContainer.js

+const updateBottomPaddingToAllowReadMoreToScrollToTop = (document, window) => {
+  const div = document.getElementById('pagelib_footer_container_ensure_can_scroll_to_top')
+  let currentPadding = parseInt(div.style.paddingBottom, 10)
+  if (isNaN(currentPadding)) { currentPadding = 0 }


Since NaN is falsy, I think the preceeding line could just be currentPadding = parseInt(...) || 0

niedzielski · 2017-07-14T14:56:39Z

src/transform/FooterContainer.js

+  const div = document.getElementById('pagelib_footer_container_ensure_can_scroll_to_top')
+  let currentPadding = parseInt(div.style.paddingBottom, 10)
+  if (isNaN(currentPadding)) { currentPadding = 0 }
+  const height = div.clientHeight - currentPadding


Do we need to check border width and margin here too?

niedzielski · 2017-07-14T14:58:44Z

src/transform/FooterMenu.js

+    fragment.appendChild(tables[i].cloneNode(true))
+  }
+  // Remove some element so their text doesn't appear when we use "innerText"
+  Array.from(fragment.querySelectorAll('.hide-when-compact, .collapsed')).forEach(el => el.remove())


Since I ended up with a lot of these in my own code, I ended up adding a polyfill in my patch for querySelectorAll.

I copied your polyfill method over and used it :)

niedzielski · 2017-07-14T15:44:35Z

src/transform/FooterMenu.js

+
+/**
+ * Type representing kinds of menu items.
+ * @type {Object}


I'm still fuzzy on how these @type / @typedef are intended to work but I think this should be @type {!MenuItemType} and in another comment you'd do @typedef {{name: !string, id: !number}} MenuItemType or

@typedef {object} MenuItemType @var {!number} languages @var {!number} lastEdited

How about this? Fixed if that seems ok.

niedzielski · 2017-07-14T15:52:38Z

src/transform/FooterMenu.js

+   * @return {!string}
+   */
+  iconClass() {
+    switch (this.itemType) {


An alternative to this switch might be:

class MenuItemType { constructor(id, iconClass) { this._id = id this._iconClass = iconClass } get id() { return this._id } get iconClass() { return this._iconClass } } const MENU_ITEMS = { LANGUAGES: new MenuItemType(1, 'pagelib-footer-menu-icon-languages'), LAST_EDITED: new MenuItemType(2, 'pagelib-footer-menu-icon-last-edited'), PAGE_ISSUES: new MenuItemType(3, 'pagelib-footer-menu-icon-page-issues'), DISAMBIGUATION: new MenuItemType(4, 'pagelib-footer-menu-icon-disambiguation'), COORDINATE: new MenuItemType(5, 'pagelib-footer-menu-icon-coordinate') }

Then the WMFMenuItem.iconClass() implementation becomes return menuItem.iconClass() and you don't have to remember to update it when adding new types.

I chewed on this for a bit... thinking I'd prefer to keep MenuItemType an enum. Also was curious as to the reason for opposition to switch... Thoughts?

The reason is the elimination of conditional logic and the potential to fall out of sync between the conditional and the declaration, adding new entries is one line instead of a couple, and everything that embodies a menu item's distinction from other items is in one place.

WRT the discussion on how payloadExtractor() would look after this change, you just tack on the ID getter on to the switch and cases (if it's not possible to also embed this conditional into MenuItemType itself):

payloadExtractor() { switch (this.itemType.id) { case MenuItemType.pageIssues.id: return pageIssuesStringsArray case MenuItemType.disambiguation.id: return disambiguationTitlesArray default: return undefined } }

If it's ok I'd prefer to consider internals refactors in follow-ups. Preferably after I've had a chance to add a few more tests.

niedzielski · 2017-07-14T15:53:21Z

src/transform/FooterMenu.js

+/**
+ * Menu item model.
+ */
+class WMFMenuItem {


Everything in the library will probably be WMF-ish. Any reason not to call this just "MenuItem"?

niedzielski · 2017-07-14T15:55:01Z

src/transform/FooterReadMore.js

+ * @param {!string} headingID
+ * @param {!Document} document
+ */
+const setHeading = (headingString, headingID, document) => {


Consider consistently placing the document / window parameter in the same position in each method signature unless there's a good reason to deviate.

niedzielski · 2017-07-14T15:56:22Z

src/transform/FooterReadMore.js

+   * @return {void}
+   */
+
+let _saveForLaterString = null


These globals suggest that a class implementation may be more appropriate. The client can than keep one or more instances of that class as needed.

Good news! I didn't need those globals after all. We were already calling a method from native land to set the save button icon state - I modified it to be passed the text to use for the current state of the save button at the same time.

So...fixed!

niedzielski · 2017-07-18T03:46:23Z

src/transform/FooterMenu.css

@@ -0,0 +1,63 @@
+
+.pagelib_footer_menu_item {
+    padding: 10px;


Would you mind if we stuck with two-space indentation for CSS too?

niedzielski · 2017-07-18T03:48:07Z

src/transform/FooterMenu.js

+ * @typedef {number} MenuItemType
+ */
+
+// eslint-disable-next-line


Please limit this to the rules you want to disable. e.g., eslint-disable-next-line valid-jsdoc.

Ah will do.

niedzielski · 2017-07-18T04:09:45Z

src/transform/FooterMenu.js

+  for (let i = 0; i < tables.length; i++) {
+    fragment.appendChild(tables[i].cloneNode(true))
+  }
+  // Remove some element so their text doesn't appear when we use "innerText"


Because you're using a DocumentFragment, you have to manually walk clone and walk this list to remove hidden elements? Would using window.document allow you to avoid the clone and selective deletion? According to the MDN documentation, Node.innerText (as opposed to Node.textContent): "innerText is aware of style and will not return the text of hidden elements, whereas textContent will."

https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent

iirc I tried that. I'll add some tests with the tricky cases I found then we can safely try alternatives.

niedzielski · 2017-07-18T04:22:36Z

src/transform/FooterReadMore.css

+  margin-bottom: 10px;
+  border-radius: 2px;
+  box-shadow: 0px 2px 4px rgba(200, 204, 209, 0.5);
+  display:block;


Missing a space here between display and block :]

niedzielski · 2017-07-18T04:23:53Z

src/transform/FooterReadMore.css

+}
+
+.pagelib_footer_readmore_page_title {
+  color: #222222;


Would you mind if we used #rgb[a] notation here? #222

niedzielski · 2017-07-18T04:24:33Z

src/transform/FooterReadMore.css

+}
+
+.pagelib_footer_readmore_bookmark_unfilled {
+    background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADAAAABGCAYAAACKeRdbAAAABGdBTUEAALGPC/xhBQAAAAFzUkdCAK7OHOkAAAAgY0hSTQAAeiYAAICEAAD6AAAAgOgAAHUwAADqYAAAOpgAABdwnLpRPAAAAAZiS0dEAP8A/wD/oL2nkwAABIdJREFUaN7tmltMVEcYgL+zV1AU5KJAFSkg64OtNd5SW3vRhFTTRGuTXrXxAktFRaWYNNqkapt0DaKlFaWUpNE2PtnENjZprS82JhqVJkRj3QVKIaAg5bqwN3b39OG4RwyiK7KMpud72pmdM/N//+yZM7M50lzrZZknGJ3oAP73AobBhcvfzBUdT1jMy69WPz/xM6AJiEYTEI0mIBpNQDSagGg0AdFoAqLRBESjCYhGExCNJiAaTUA0moBoNAHRaAKi0QREowmIRhMQjSYwHLIMtuNNfHasETmC7wIYHr2LoQSC8Ol3Dfx6sRMAjy/I3vVPo49AukZdwOeX+bjyb/6o6VbrfrvUiccXxGbNwGiQRnW8Uc2J2xuk8Kvau4IPcbamm+3ldXh8wcdTwOkKsPGgg8t2p1q3fnkK65alqOUL13op/LoOl3f0JEZFoNPpx1rq4GpDv1pX+OZUClaksmllKhtXPKXW/+lwUnDQgdMVeDwEWjt95JbYqW12KR3qJHauns4HOVPUNhuWJ1P01jS1fLWhH2upgy6nX6xAc7uX3BI7TW0eAAx6ib3r0lm1OHFI2/eWTmbX6unodMpNXNvsIm+/nfbuATEC9Tc85JbYae30AWAy6tiXn8lrC+KHveaNxYnsWZuO/rbEP60e8vbbudnhG1uB600u8kvt/NujZC/KpOPLzVm8PDv2gdcuWxjPF9YMDHpFQp3FW96xEaip7+fDAw66+5Tfb0y0nvJt2SyYOSHsPpbMiaO0IAuzURm+rctHXomduhZ3ZAUuXXeyuayWPreygsTFGKgoymZ25viHHviFWRMp25JFtFkPQEfvAPmlDv5qdEVG4NyVHrYeqsPtVYJPjDVSWWxhZtq4hw4+xDzLBA5vm0FMtCLR0+9n40EHNfX9YfcRlsCZ6i6Kj9TjG1AeQCkJZqp2WMhIiRpx8CGeyRhPRVE2cTHKrqbPHWBzWS0XrzvDuv6BAqfOd7CrqgF/QNlSpk2JomqHhalJ5kcOPsTMtHFUFltIjDUC4PYG2HaojnNXeh5N4MTZdvYcbSQQVILPTI3m22ILUyYZRy34EBkpSmKS400A+AaCFB+p50x118gEvj/dhu14E/LtzXwoSwkTI7IDB2Bqkvmu2fUHZHZVNXDqfMfDCVT9cpOyH5vV8rOZMVQUZRM7Xh+x4EMkx5uU+ys1GoBAUGbP0UZOnG0PT6D85A0qfr6hludZJlC+9c5KMRYkxhqp/ChbXeFkWcZ2vIkffm8b0lafOte6O1To9wQ5drpV/XLRrFgObMoiyjT2R+cok46c+fFUO/q41aVsNS5c60WSJKodd1Yoabi31195Lg7boEe+KNzeINvL6+46ZwzmnqnNmR/PvvxM4cEDRJt1lG2ZwaJZ995nDRF4/fkEPt8QmQP4SDEbJQ4UZPLqnEn3F1j1UhK716ajE5/4IRj0EjZrBssWJtxb4J0lk9n5fproOO+LXgd716Wz8sU7ByYdwJqcZIrfnjbijscSSYJP1kzn3aW3j6xHfmqRn1TKT7bIkixH8o+/yPMfGVAsFZFJcHoAAAAASUVORK5CYII=);


The indent is inconsistent here.

niedzielski · 2017-07-18T04:24:46Z

src/transform/FooterReadMore.css

+}
+
+.pagelib_footer_readmore_page_save {
+  color: #2C5BC5;


One more lowercase case :]

niedzielski · 2017-07-18T04:26:10Z

src/transform/FooterReadMore.js

+ * @return {void}
+ */
+
+const _saveButtonIDPrefix = 'readmore:save:'


This looks like a static immutable constant, would you mind if we stuck with UPPER_CASE-like syntax for these. So, SAVE_BUTTON_ID_PREFIX? I guess our functions are also constants but this is constant data at file scope.

Not big on upper-casing WHYAREYOUSHOUTING but I like consistency so will do :)

niedzielski · 2017-07-18T04:28:00Z

src/transform/FooterReadMore.js

+  if (readMorePage.terms) {
+    description = readMorePage.terms.description[0]
+  }
+  if ((description === undefined || description.length < 10) && readMorePage.extract) {


This could be concisely written as (description || '').length < 10 or (!description || description.length < 10).

Ooh nice! I'll try that.

niedzielski · 2017-07-18T04:30:42Z

src/transform/FooterReadMore.js

+      }
+    }
+  }
+  xhr.onerror = e => {


The curly braces do not appear to add any value here and the parameter is unused.

niedzielski · 2017-07-18T04:33:13Z

src/transform/FooterReadMore.js

+  const unfilledClass = 'pagelib_footer_readmore_bookmark_unfilled'
+  const filledClass = 'pagelib_footer_readmore_bookmark_filled'
+  button.classList.remove(unfilledClass)
+  button.classList.remove(filledClass)


Since add won't doubly add, consider reducing this to: button.classList.remove(isSaved ? unfilledClass : filledClass).

https://developer.mozilla.org/en-US/docs/Web/API/Element/classList

Oh sweet good catch!!

Turns out classList remove can be passed multiple class names. I used that rather than adding another ternary.

niedzielski · 2017-07-19T03:01:06Z

👍 published as v4.3.0. Please update your integration patch to reference this version (and don't forget to unlink before building the bundle).

montehurd · 2017-07-19T17:22:15Z

Please update your integration patch to reference this version (and don't forget to unlink before building the bundle)

hehe that's got me a few times :)

montehurd added 2 commits July 12, 2017 14:56

Add footer legal transform.

3e34f1f

Add footer container transform.

0e82cb2

montehurd added the WIP label Jul 12, 2017

montehurd requested a review from niedzielski July 12, 2017 23:54

montehurd mentioned this pull request Jul 12, 2017

Integrate pagelib footer transforms wikimedia/wikipedia-ios#1615

Merged

5 tasks

Add footer menu transform.

632786b

niedzielski reviewed Jul 13, 2017

View reviewed changes

montehurd commented Jul 13, 2017

View reviewed changes

montehurd added 3 commits July 13, 2017 17:20

Add footer read more transform.

d21130b

Use 'pagelib' prefix on footer transform css class names and element …

96dcc5e

…ids.

Fix typo.

6be7ba4

niedzielski reviewed Jul 14, 2017

View reviewed changes

niedzielski reviewed Jul 18, 2017

View reviewed changes

montehurd added 9 commits July 18, 2017 12:23

classList remove can be passed multiple class names.

7caf6a8

Remove unused error parameter.

3f90f93

Simplify description heuristic.

f2b99b9

Use upper case for static immutable const.

a548f88

Use valid-jsdoc linting exception specifier.

b814ee7

Use single quotes.

62282ff

Use const for extractor.

32ab8a2

Add first read more tests.

b09b8dc

Remove unneeded bits from switch.

a265f98

montehurd removed the WIP label Jul 18, 2017

montehurd added 2 commits July 18, 2017 17:11

Manually lint some CSS.

8aebb6b

Remove blank first and last CSS file lines.

d964504

montehurd changed the title ~~Footer transforms~~ New: Footer transforms Jul 19, 2017

niedzielski added 2 commits July 18, 2017 21:56

Doc: update changelog for footer transforms (v4.3.0)

8880493

Merge: master into footerTransforms

2147360

niedzielski merged commit af78b3a into master Jul 19, 2017

niedzielski deleted the footerTransforms branch July 19, 2017 02:59

New: Footer transforms #53

New: Footer transforms #53

Conversation

montehurd commented Jul 12, 2017 • edited

montehurd commented Jul 13, 2017

niedzielski commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedzielski Jul 14, 2017 • edited

Choose a reason for hiding this comment

montehurd Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montehurd Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedzielski Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niedzielski commented Jul 19, 2017

montehurd commented Jul 19, 2017

montehurd commented Jul 12, 2017 •

edited

niedzielski Jul 14, 2017 •

edited

montehurd Jul 15, 2017 •

edited

montehurd Jul 15, 2017 •

edited

niedzielski Jul 18, 2017 •

edited