WIP : multi panels (for question comment purpose)

[kosamari]

Just making PR because commenting UI is better on PR.

Approach I took was to accept any HTML elements as children of <multi-panels>

<multi-panels> will simply take even indexed elements as heading, and element following(thus odd index) as content. Allowing user of this component more freedom in markup.

note: I don't really understand TypeScript(yet) and this might very well be not great code(yet) !

[kosamari]

I don't know what to do with types when we are doing simple event listeners...
When I do

_onClick(event : Event) {
  const heading : Element = event.target
}

then type script yells Type 'EventTarget | null' is not assignable to type 'Element'

[jakearchibald]

You can go one of two ways with this. If you think event.target can only ever be an Element, you can do:

const heading = event.target as Element;

See https://www.typescriptlang.org/docs/handbook/basic-types.html#type-assertions.

However, if TypeScript is being helpful, you can make use of type guards.

if (!(event.target instanceof Element)) return;
const heading: Element = event.target;

The above works, because TypeScript infers that event.target must be an Element.

[kosamari]

Type script is yelling Property 'focus' does not exist on type 'Element' here... but I can't find the way to make return of _allHeadings to HTMLElement ...

[kosamari]

If we want to support IEs and FF before 37, then Spacebar might be needed here.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values

[jakearchibald]

We don't need to support those browsers for this project, but in terms of using this component outside this project, I'll leave it up to you.

[kosamari]

handled by adding case 'Spacebar':

[kosamari]

switched to using event.key per @mathiasbynens's suggestion on previouse commit :)

[mathiasbynens]

Nice work — I love it! So readable 👍🏻

[mathiasbynens]

Nit: use forEach instead of a hand-written loop. forEach is highly optimized for proper arrays in V8, so it’s likely faster, and it still gives you access to the index variable (unlike for-of).

[mathiasbynens]

(https://v8project.blogspot.com/2017/09/elements-kinds-in-v8.html if you’re interested in the background)

[jakearchibald]

Hah, I recommended for-of below. Which is better? Fwiw I think we should be optimising for readability over what Chrome does fastest on a particular day.

[mathiasbynens]

Fwiw I think we should be optimising for readability over what Chrome does fastest on a particular day.

To be clear, I fully agree with this, and this matches our general V8 messaging.

In this case, a handwritten for loop expresses implementation, whereas for-of + entries or forEach would express intent. Expressing intent often leads to more readable code, IMHO.

forEach likely being faster than for in this case is an indirect result of that (and a nice little bonus). By expressing intent rather than implementation, you generally give the JS engine more information to work with, often leading to greater behind-the-scenes optimization potential.

[mathiasbynens]

Maybe document the edge cases here? E.g. when headings contains a single element, this code just returns that.

[jakearchibald]

Our target browsers support:

for (const [i, child] of children.entries())

I'm undecided if this is better or not.

[mathiasbynens]

Between that or forEach((child, index) => { … }), I don’t really have a preference. I do think either of those are nicer than the hand-written for loop though

[jakearchibald]

No preference here either.

[developit]

Why use an Array at all?

let child = this.firstElementChild;
while (child) {
  // stuff
  child = child.nextElementSibling;
}

• nice balance of readability and performance
• avoids the array casting entirely

[mathiasbynens]

Array operations are heavily optimized in V8 for actual arrays. The one-off cost of casting to an array is usually worth the later performance benefits when operating on the array (as opposed to an array-like object). (shameless https://www.youtube.com/watch?v=m9cTaYI95Zc plug in case anyone wants more info)

TL;DR Don’t think of the Array.from() as overhead.

[jakearchibald]

I'm unsure about this as a fix. Imagine code like this:

multiPanel.append(parse(`<h1>Hello!</h1>`));
const data = await fetch(content).then(r => r.json());
multiPanel.append(parse(data.html));

In the above the developer is adding a heading, then going to the network to fetch the content. However, you're going to add an extra div after their heading, meaning their 'content' is going to be treated as a heading.

[jakearchibald]

You can just add these in the constructor.

[jakearchibald]

If you add the listeners in the constructor, you don't need to remove them here.

[jakearchibald]

The following code assumes the internals of the multi-panel are complete once the element is connected, and never change after that. Ideally that shouldn't be assumed. It'd be better to observe this stuff in _childrenChange and react to elements coming and going.

[jakearchibald]

If the child already has an ID we shouldn't overwrite it.

[jakearchibald]

Ohh, and you're going to get duplication of IDs if you have two <multi-panel>s. Maybe generate a random ID?

[jakearchibald]

Is https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details a better primitive here in terms of accessibility? @surma did you ever look into this?

It might be that <details> buggers up too much when it comes to animation.

[jakearchibald]

This breaks if the user clicks an element inside the heading. As in:

<h1>Hello <em>world</em></h1>

Clicking world won't work.

Instead of _isHeading, maybe write something like _getClosestHeading? el.closest('multi-panel > *') will help here.

_getClosestHeading should return undefined/null if the user didn't click within a heading.

[jakearchibald]

Might be worth looking at user defined type guards here.

That way, you can tell TypeScript that, if _isHeading returns true, the type of currentHeading must be HTMLElement or whatever.

[jakearchibald]

Note: if you do the _closestHeading thing I mentioned above, _isHeading is still the best thing to use in this case.

[jakearchibald]

Good catch!

[jakearchibald]

This will also pick up heading of multi-selects inside this one. this.querySelectorAll(':scope > .panel-heading') would prevent this.

[jakearchibald]

It'd be more efficient to use currentHeading.previousElementSibling to find the previous heading, as you don't have to look up all headings.

[jakearchibald]

I think this would be faster:

return this.firstElementChild;

[jakearchibald]

I think this would be faster:

const children = this.children;
if (children.length % 1) {
  // Odd number of children. Last one must be a heading.
  return children[children.length - 1];
}
// Otherwise, the second-last child is the heading.
return children[children.length - 2];

[kosamari]

Status :
Current code has all refraction of the previous code review (minus <details> consideration).

This component currently assumes there is always a set of heading and content.
and expands corresponding content via nextElementSibling.

If developer using this component does something like below (per @jakearchibald's review) and user click on heading before content is created, it should be fine (simply nonextElementSibling)

multiPanel.append(parse(`<h1>Hello!</h1>`));
const data = await fetch(content).then(r => r.json());
multiPanel.append(parse(data.html));

however, if developer inserts element in async way like above in the middle of multi panel children... it will mess up which panel to be 'expanded'.

Not sure expecting dev user to "always append 2 elements together" is a reasonable expectation (code will be simpler !) or we should bullet proof this with unique ID approach and control element gets expanded based on ID.

note: currently unique id is assigned just for A11y but not for control.

[jakearchibald]

I think this would be more readable as:

for (let i = 0; i < children.length; i += 2) {
  const heading = children[i];
  const content = children[i+1];
  const randomId = Math.random().toString(36).substr(2, 9);

  // …
}

As you don't need to do the odd/even check, and randomId gets better scoping.

[kosamari]

handled this in suggested way.

[jakearchibald]

Earlier?

[jakearchibald]

If there's an if statement with a short part & a long part, I think it's more readable to handle the short part first. In some cases it means you can avoid the else. In this case:

if (child.classList.contains('panel-heading')) {
  tabIndexAlreadySet = true;
  return;
}

child.classList.add('panel-heading');
// …

The return will need to be continue if you change this to a for-loop.

[jakearchibald]

I think this strategy has problems if you replace just a heading, or just the content. Or, if you swap some headings around but not the content.

[kosamari]

closing this in lieu of #95