Skip to content
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

WIP : multi panels (for question comment purpose) #24

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 172 additions & 1 deletion components/MultiPanel/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,183 @@
import './styles.css';

/**
* A multi-panel view that the user can add any number of 'panels'.
* 'a panel' consists of two elements. even index element begomes heading,
* and odd index element becomes the expandable content.
*/
export default class MultiPanel extends HTMLElement {

constructor () {
super();

// Watch for children changes.
new MutationObserver(() => this._childrenChange())
.observe(this, { childList: true });
}

connectedCallback () {
this._childrenChange();

const children : Element[] = Array.from(this.children);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (let i : number = 0; i < children.length; i++){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const child : Element = children[i]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our target browsers support:

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

I'm undecided if this is better or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No preference here either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if(i % 2 === 0){
// even index = heading element
child.classList.add('panel-heading');

// for A11y
child.id = `panel-heading-${i}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

child.setAttribute('tabindex', '-1');
child.setAttribute('aria-controls', `panel-content-${i}`);
} else {
// odd index = content element
child.classList.add('panel-content');

// for A11y
child.id = `panel-content-${i-1}`;
child.setAttribute('aria-labelledby', `panel-heading-${i-1}`);
}
}

// make the first heading focusable.
children[0].setAttribute('tabindex', '0');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// add EventListners
this.addEventListener('click', this._onClick);
this.addEventListener('keydown', this._onKeyDown);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just add these in the constructor.

}

// remove EventListeners
disconnectedCallback() {
this.removeEventListener('click', this._onClick);
this.removeEventListener('keydown', this._onKeyDown);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Click event handler
_onClick(event) {
const heading : Element = event.target;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (this._isHeading(heading)) {
this._expand(heading);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// KeyDown event handler
_onKeyDown(event) {
const currentHeading : Element = event.target;

// if clicke event is not on heading element, ignore
if (!this._isHeading(currentHeading)) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// don’t handle modifier shortcuts used by assistive technology.
if (event.altKey) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

let newHeading;
switch (event.key) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work — I love it! So readable 👍🏻

case 'ArrowLeft':
case 'ArrowUp':
newHeading = this._prevHeading();
break;

case 'ArrowRight':
case 'ArrowDown':
newHeading = this._nextHeading();
break;

case 'Home':
newHeading = this._firstHeading();
break;

case 'End':
newHeading = this._lastHeading();
break;

case 'Enter':
case ' ':
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handled by adding case 'Spacebar':

if (this._isHeading(document.activeElement)) {
this._expand(document.activeElement);
}
break;

// Any other key press is ignored and passed back to the browser.
default:
return;
}

event.preventDefault();
currentHeading.setAttribute('tabindex', '-1');
if (newHeading) {
newHeading.setAttribute('tabindex', '0');
newHeading.focus();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

}
}

_expand (heading : Element) {
const content = heading.nextElementSibling;

// heading elment should always have nextElementSibling (checked on _childrenChange)
// but in case it is null, return.
if (content === null) {
return;
}

// toggle expanded and aria-expanded attoributes
if (content.hasAttribute('expanded')) {
content.removeAttribute('expanded')
content.setAttribute('aria-expanded', 'false');
} else {
content.setAttribute('expanded', '')
content.setAttribute('aria-expanded', 'true');
}
}

// children of multi-panel should always in even nuber (heading/content pair)
// if children are odd numbers, add a div at the end to prevent potential error.
_childrenChange () {
if (this.children.length % 2 !== 0) {
console.error('detected odd number of elements inside multi-panel, please make sure you have heading/content pair')
this.appendChild(document.createElement('div'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

// called to check if an element passed is panel heading
_isHeading (heading : Element) {
return heading.classList.contains('panel-heading');
}

// return list of panel heading elements
_allHeadings () {
return Array.from(this.querySelectorAll('.panel-heading'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// returns headding that is before currently selected one.
_prevHeading () {
const headings : Element[] = this._allHeadings();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let newIdx : number = headings.findIndex(headings => headings === document.activeElement) - 1;
return headings[(newIdx + headings.length) % headings.length];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// returns headding that is after currently selected one.
_nextHeading () {
const headings : Element[] = this._allHeadings();
let newIdx : number = headings.findIndex(heading => heading === document.activeElement) + 1;
return headings[newIdx % headings.length];
}

// returns first heading in multi-panel.
_firstHeading () {
const headings : Element[] = this._allHeadings();
return headings[0];
Copy link
Collaborator

@jakearchibald jakearchibald Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be faster:

return this.firstElementChild;

}

// returns last heading in multi-panel.
_lastHeading () {
const headings : Element[] = this._allHeadings();
return headings[headings.length - 1];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];

}
}

customElements.define('multi-panel', MultiPanel);
customElements.define('multi-panel', MultiPanel);
12 changes: 10 additions & 2 deletions components/MultiPanel/styles.css
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
multi-panel {

multi-panel > .panel-heading {
background:gray;
}
multi-panel > .panel-content {
height:0px;
overflow:scroll;
transition: height 1s;
}
multi-panel > .panel-content[expanded] {
height:30px;
}
9 changes: 8 additions & 1 deletion components/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ import '../MultiPanel/index';

document.body.innerHTML = `
<h1>Multi panel</h1>
<multi-panel>TODO</multi-panel>
<multi-panel>
<div>Heading 1</div>
<div>panel content 1 panel content 1 panel content 1</div>
<div>Heading 2</div>
<div>panel content 2 panel content 2 panel content 2</div>
<div>Heading 3</div>
<div>panel content 3 panel content 3 panel content 3</div>
</multi-panel>
<h1>Side by side</h1>
<two-up legacy-clip-compat>
<pinch-zoom>
Expand Down