Merge pull request #609 from ing-bank/chore/cleanTodos

chore: cleanup todos

packages/ajax/src/interceptors.js

@@ -1,6 +1,5 @@
 import { axios } from '@bundled-es-modules/axios';
 
-// FIXME: lang must be dynamic, fallback to html tag lang attribute or use the user-provided one
 export function addAcceptLanguageHeaderInterceptorFactory(lang) {
   return config => {
     const result = config;

packages/button/src/LionButton.js

@@ -239,7 +239,6 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
       e.preventDefault();
     }
 
-    // FIXME: In Edge & IE11, this toggling the active state to prevent bounce, does not work.
     this.active = true;
     const keyupHandler = keyupEvent => {
       if (isKeyboardClickEvent(keyupEvent)) {

packages/choice-input/src/ChoiceGroupMixin.js

@@ -23,9 +23,6 @@ export const ChoiceGroupMixin = dedupeMixin(
       get modelValue() {
         const elems = this._getCheckedElements();
         if (this.multipleChoice) {
-          // TODO: holds for both modelValue and serializedValue of choiceInput:
-          // consider only allowing strings as values, in which case 'el.value' would suffice
-          // and choice-input could be simplified
           return elems.map(el => el.modelValue.value);
         }
         return elems[0] ? elems[0].modelValue.value : '';
@@ -79,8 +76,6 @@ export const ChoiceGroupMixin = dedupeMixin(
        */
       addFormElement(child, indexToInsertAt) {
         this._throwWhenInvalidChildModelValue(child);
-        // TODO: nice to have or does it have a function (since names are meant as keys for
-        // formElements)?
         this.__delegateNameAttribute(child);
         super.addFormElement(child, indexToInsertAt);
       }

packages/fieldset/src/FormGroupMixin.js

@@ -95,11 +95,6 @@ export const FormGroupMixin = dedupeMixin(
         this._setValueMapForAllFormElements('formattedValue', values);
       }
 
-      // TODO: should it be any or every? Should we maybe keep track of both,
-      // so we can configure feedback visibility depending on scenario?
-      // Should we allow configuring feedback visibility on validator instances
-      // for maximal flexibility?
-      // Document this...
       get prefilled() {
         return this._everyFormElementHas('prefilled');
       }

packages/fieldset/test/lion-fieldset.test.js

@@ -64,7 +64,6 @@ describe('<lion-fieldset>', () => {
   });
 
   // TODO: Tests below belong to FormRegistrarMixin. Preferably run suite integration test
-
   it(`${tagString} has an up to date list of every form element in .formElements`, async () => {
     const el = await fixture(html`<${tag}>${inputSlots}</${tag}>`);
     await nextFrame();

packages/input-amount/src/LionInputAmount.js

@@ -98,7 +98,7 @@ export class LionInputAmount extends LocalizeMixin(LionInput) {
    * @override of FormatMixin
    */
   __callParser(value = this.formattedValue) {
-    // TODO: input and change events both trigger parsing therefore we need to handle the second parse
+    // TODO: (@daKmor) input and change events both trigger parsing therefore we need to handle the second parse
     this.__parserCallcountSincePaste += 1;
     this.__isPasting = this.__parserCallcountSincePaste === 2;
     this.formatOptions.mode = this.__isPasting === true ? 'pasted' : 'auto';
@@ -122,7 +122,7 @@ export class LionInputAmount extends LocalizeMixin(LionInput) {
   }
 
   __setCurrencyDisplayLabel() {
-    // TODO: for optimal a11y, abbreviations should be part of aria-label
+    // TODO: (@erikkroes) for optimal a11y, abbreviations should be part of aria-label
     // example, for a language switch with text 'en', an aria-label of 'english' is not
     // sufficient, it should also contain the abbreviation.
     this._currencyDisplayNode.setAttribute('aria-label', getCurrencyName(this.currency));

packages/input-datepicker/src/LionCalendarOverlayFrame.js

@@ -38,7 +38,6 @@ export class LionCalendarOverlayFrame extends LocalizeMixin(LitElement) {
   static get localizeNamespaces() {
     return [
       {
-        /* FIXME: This awful switch statement is used to make sure it works with polymer build.. */
         'lion-calendar-overlay-frame': locale => {
           switch (locale) {
             case 'bg-BG':

packages/input-datepicker/src/LionInputDatepicker.js

@@ -57,7 +57,6 @@ export class LionInputDatepicker extends OverlayMixin(LionInputDate) {
   static get localizeNamespaces() {
     return [
       {
-        /* FIXME: This awful switch statement is used to make sure it works with polymer build.. */
         'lion-input-datepicker': locale => {
           switch (locale) {
             case 'bg-BG':
@@ -206,6 +205,8 @@ export class LionInputDatepicker extends OverlayMixin(LionInputDate) {
    */
   _overlayTemplate() {
     // TODO: add performance optimization to only render the calendar if needed
+    // When not opened (usually on init), it does not need to be rendered.
+    // This would make first paint quicker
     return html`
       <lion-calendar-overlay-frame>
         <span slot="heading">${this.calendarHeading}</span>
@@ -245,8 +246,6 @@ export class LionInputDatepicker extends OverlayMixin(LionInputDate) {
    */
   // eslint-disable-next-line class-methods-use-this
   _invokerTemplate() {
-    // TODO: aria-expanded should be toggled by Overlay system, to allow local overlays
-    // (a.k.a. dropdowns) as well. Important: will be breaking for subclassers
     return html`
       <button
         type="button"

packages/input-datepicker/test-helpers/DatepickerInputObject.js

@@ -1,6 +1,5 @@
 import { CalendarObject } from '@lion/calendar/test-helpers.js';
 
-// TODO: refactor CalendarObject to this approach (only methods when arguments are needed)
 export class DatepickerInputObject {
   constructor(el) {
     this.el = el;

packages/input-datepicker/test/lion-input-datepicker.test.js

@@ -29,10 +29,6 @@ describe('<lion-input-datepicker>', () => {
       ).not.to.equal(null);
     });
 
-    it.skip('activates full screen mode on mobile screens', async () => {
-      // TODO: should this be part of globalOverlayController as option?
-    });
-
     it('has a close button, with a tooltip "Close"', async () => {
       const el = await fixture(html`
         <lion-input-datepicker></lion-input-datepicker>
@@ -72,7 +68,6 @@ describe('<lion-input-datepicker>', () => {
       ).lightDom.to.equal('foo');
     });
 
-    // TODO: fix the Overlay system, so that the backdrop/body cannot be focused
     it('closes the calendar on [esc] key', async () => {
       const el = await fixture(html`
         <lion-input-datepicker></lion-input-datepicker>
@@ -130,10 +125,6 @@ describe('<lion-input-datepicker>', () => {
       expect(elObj.overlayController.isShown).to.equal(true);
     });
 
-    // Relies on delegation of disabled property to invoker.
-    // TODO: consider making this (delegation to interactive child nodes) generic functionality
-    // inside LionField/FormControl. Or, for maximum flexibility, add a config attr
-    // to the invoker node like 'data-disabled-is-delegated'
     it('delegates disabled state of host input', async () => {
       const el = await fixture(html`
         <lion-input-datepicker disabled></lion-input-datepicker>

packages/localize/src/LocalizeManager.js

@@ -226,7 +226,6 @@ export class LocalizeManager extends LionSingleton {
     return locale.substring(0, 2);
   }
 
-  // TODO: this method has to be removed when EventTarget polyfill is available on IE11
   _fakeExtendsEventTarget() {
     const delegate = document.createDocumentFragment();
     ['addEventListener', 'dispatchEvent', 'removeEventListener'].forEach(funcName => {

packages/localize/src/date/getWeekdayNames.js

@@ -32,10 +32,6 @@ function getCachedWeekdayNames(locale) {
   return weekdayNamesCache[locale];
 }
 
-// TODO: consider using a database with information for the `firstDayOfWeek`?
-// https://github.com/unicode-cldr/cldr-core/blob/35.0.0/supplemental/weekData.json#L60
-// https://github.com/tc39/ecma402/issues/6#issuecomment-114079502
-
 /**
  * @desc Returns weekday names for locale
  * @param {string} options.locale locale

packages/overlays/src/OverlayController.js

@@ -120,7 +120,6 @@ export class OverlayController {
     return this._contentNodeWrapper.zIndex;
   }
 
-  // TODO: Use deepmerge package for doing this kind of config merging...
   /**
    * @desc Allows to dynamically change the overlay configuration. Needed in case the
    * presentation of the overlay changes depending on screen size.
@@ -187,7 +186,6 @@ export class OverlayController {
       // Now, it is time to lazily load Popper if not done yet
       // Do we really want to add display: inline or is this up to user?
       if (!this.constructor.popperModule) {
-        // TODO: Instead, prefetch it or use a preloader-manager to load it during idle time
         this.constructor.popperModule = preloadPopper();
       }
     }
@@ -204,7 +202,6 @@ export class OverlayController {
     }
   }
 
-  // FIXME: Consider that state can also be shown (rather than only initial/closed), and don't hide in that case
   /**
    * @desc Cleanup ._contentNodeWrapper. We do this, because creating a fresh wrapper
    * can lead to problems with event listeners...
@@ -243,12 +240,12 @@ export class OverlayController {
   }
 
   __initAccessibility() {
-    // TODO: add setup props in object and restore on teardown
+    // TODO: remove a11y attributes on teardown
     if (!this.contentNode.id) {
       this.contentNode.setAttribute('id', this._contentId);
     }
     if (this.isTooltip) {
-      // TODO: this could also be labelledby
+      // TODO: (@tlouisse) this could also be labelledby.
       if (this.invokerNode) {
         this.invokerNode.setAttribute('aria-describedby', this._contentId);
       }
@@ -624,7 +621,6 @@ export class OverlayController {
     });
   }
 
-  // TODO: this method has to be removed when EventTarget polyfill is available on IE11
   __fakeExtendsEventTarget() {
     const delegate = document.createDocumentFragment();
     ['addEventListener', 'dispatchEvent', 'removeEventListener'].forEach(funcName => {

packages/overlays/src/OverlayMixin.js

@@ -149,7 +149,8 @@ export const OverlayMixin = dedupeMixin(
           return this._cachedOverlayContentNode;
         }
 
-        // FIXME: This should shadow outlet in between the host and the content slot, is a problem
+        // (@jorenbroekema) This should shadow outlet in between the host and the content slot,
+        // is a problem.
         // Should simply be Array.from(this.children).find(child => child.slot === 'content')
         // Issue: https://github.com/ing-bank/lion/issues/382
         const shadowOutlet = Array.from(this.children).find(

packages/overlays/test/OverlayController.test.js

@@ -917,7 +917,7 @@ describe('OverlayController', () => {
       expect(ctrl.contentNode).to.equal(contentNode);
     });
 
-    // TODO: Currently not working, enable again when we fix updateConfig
+    // Currently not working, enable again when we fix updateConfig
     it.skip('allows for updating viewport config placement only, while keeping the content shown', async () => {
       const contentNode = fixtureSync(html`
         <div>my content</div>

packages/overlays/test/global-positioning.test.js

@@ -24,18 +24,6 @@ describe('Global Positioning', () => {
       expect(overlays.globalRootNode.children.length).to.equal(1);
       expect(overlays.globalRootNode.children[0]).to.have.trimmed.text('my content');
     });
-
-    // TODO: not implemented atm. Is this needed? If so, it should be covered in a css class
-    // on a wrapping element, since it may break user styling.
-    it.skip('sets ".contentNode" styling to display flex by default', async () => {
-      const ctrl = new OverlayController({
-        ...withDefaultGlobalConfig(),
-      });
-      await ctrl.show();
-      expect(
-        window.getComputedStyle(overlays.globalRootNode.children[0]).getPropertyValue('display'),
-      ).to.equal('flex');
-    });
   });
 
   describe('viewportConfig', () => {

packages/overlays/test/local-positioning.test.js

@@ -14,33 +14,6 @@ const withLocalTestConfig = () => ({
 });
 
 describe('Local Positioning', () => {
-  describe('Nodes', () => {
-    // TODO: check if wanted/needed
-    it.skip('sets display to inline-block for contentNode by default', async () => {
-      const invokerNode = await fixture(html`
-        <div role="button" id="invoker">Invoker</div>
-      `);
-
-      const node = document.createElement('div');
-      node.innerHTML = '<div id="content">Content</div>';
-
-      const ctrl = new OverlayController({
-        ...withLocalTestConfig(),
-        contentNode: node,
-        invokerNode,
-      });
-      const el = await fixture(html`
-        <div>
-          ${ctrl.invokerNode} ${ctrl.content}
-        </div>
-      `);
-
-      await ctrl.show();
-      const contentWrapper = el.querySelector('#content').parentElement;
-      expect(contentWrapper.style.display).to.equal('inline-block');
-    });
-  });
-
   // Please use absolute positions in the tests below to prevent the HTML generated by
   // the test runner from interfering.
   describe('Positioning', () => {
@@ -214,7 +187,7 @@ describe('Local Positioning', () => {
       );
     });
 
-    // TODO: Refactor to the new system, last expect is broken at the moment (the others pass still)
+    // TODO: Reenable test and make sure it passes
     it.skip('updates placement properly even during hidden state', async () => {
       const ctrl = new OverlayController({
         ...withLocalTestConfig(),
@@ -265,7 +238,7 @@ describe('Local Positioning', () => {
       );
     });
 
-    // TODO: Refactor to the new system, last expect is broken at the moment (the others pass still)
+    // TODO: Not yet implemented
     it.skip('updates positioning correctly during shown state when config gets updated', async () => {
       const ctrl = new OverlayController({
         ...withLocalTestConfig(),