Wc 7 popover component #30

akdetrick · 2016-12-28T20:55:18Z

(marking prelim until tests are complete)

Popover menu component (wraps links into a drop down menu activated by a button)

mmcgahan · 2016-12-28T22:02:08Z

src/popover.test.jsx

-			expect(el).not.toBeNull();
-		});
+	it('exists; menu hidden by default', () => {
+		console.warn(popoverEl);


you shouldn't need to log anything in tests - they should be self-documenting

mmcgahan · 2016-12-28T22:05:17Z

src/popover.test.jsx

+
+	it('menu dismissed on option selection', () => {
+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.click(optionEls[0]);


mmcgahan · 2016-12-28T22:05:24Z

src/popover.test.jsx

+
+	it('menu is keyboard navigatable with escape key', () => {
+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.keyDown(optionEls[0], {key: 'Escape'});


mmcgahan · 2016-12-28T22:05:31Z

src/popover.test.jsx


+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.keyUp(defaultSelected, {key: 'ArrowDown'});


mmcgahan · 2016-12-28T22:25:05Z

src/Popover.jsx

+		this.closeMenu = this.closeMenu.bind(this);
+		this.handleKeyDown = this.handleKeyDown.bind(this);
+		this.handleClick = this.handleClick.bind(this);
+		this.handleBlur = this.handleBlur.bind(this);


for functional DRY kicks:

autobind(...names) { names.forEach(name => this[name] = this[name].bind(this)); } constructor(props) { this.autobind( 'toggleMenu', 'closeMenu', 'handleKeyDown', 'handleClick', 'handleBlur' ); }

we could even break out the autobind into a utility function

function bindAll(context, ...names) { names.forEach(name => context[name] = context[name].bind(context)); }

... it turns out that underscore has exactly this with _.bindAll

mmcgahan · 2016-12-28T22:35:09Z

src/Popover.jsx

+		const { handleKeyDown } = this;
+		const isActive = this.state.isActive;
+		let menu;
+		React.Children.forEach(this.props.children, function(child) {


the two uses of React.Children.forEach in this component indicate that the children prop is probably being overloaded unnecessarily - instead, you can use other props to clearly separate the Trigger and the Menu, e.g.

const trigger = <PopoverTrigger ... />; const menu = <PopoverMenu ... />; <Popover trigger={trigger} menu={menu} ... />

and you can use Popover's propTypes to ensure that the props exist and are of the required type:

{ trigger: React.PropTypes.instanceOf(PopoverTrigger).isRequired, menu: React.PropTypes.instanceOf(PopoverMenu).isRequired, }

In general, use children for arbitrary content, and use props for things that have to conform to the component's "API".

This is related to a point made by one of the React maintainers that I came across here - I think he presents it nicely.

mmcgahan · 2016-12-28T23:12:13Z

src/PopoverTrigger.jsx

+	}
+}
+
+export default PopoverTrigger;


needs propTypes

mmcgahan · 2016-12-28T23:13:29Z

src/PopoverTrigger.jsx

+			className,
+			handleClick,
+			handleKeyDown,
+			handleBlur,


The React convention is to name event handlers with an on.. prefix to match up with the prop they get assigned to, e.g. onClick={onClick} - just a tiny bit of extra consistency.

mmcgahan · 2016-12-28T23:16:27Z

src/popover.test.jsx

+	});
+
+	it('menu appears on trigger click', () => {
+		TestUtils.Simulate.click(triggerEl);


for user interaction tests, it's good practice to test the state both before and after the simulated interaction so that you can be sure that something changed. It's not quite as big a deal here because the previous unit test tests the 'before' state, but it's also best practice to keep unit tests completely independent

mmcgahan · 2016-12-28T23:16:35Z

src/popover.test.jsx

+
+	it('menu dismissed on popover blur', () => {
+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.blur(popoverEl);


this would be a good place to put an expect, between the two simulated interactions, so that you can be sure the state didn't change to the final state before the blur.

mmcgahan · 2016-12-28T23:17:14Z

src/popover.test.jsx

+		const targetSelected = optionEls[1];
+
+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.keyUp(defaultSelected, {key: 'ArrowDown'});


It's worth checking for whether the 'up' arrow works since you have explicit logic for that event as well - should help your code coverage as well

mmcgahan · 2016-12-28T23:19:12Z

sorry, i just saw that this was Prelim - apologies if some of the changes i recommended were already on the todo list!

mmcgahan · 2016-12-29T17:57:37Z

src/popover.test.jsx

+		const triggerEl = ReactDOM.findDOMNode(trigger);
+
+		TestUtils.Simulate.click(triggerEl);
+		TestUtils.Simulate.keyUp(defaultSelected, {key: 'ArrowDown'});


I think my previous comment got lost, but just to keep it visible: you should call expect before and after every simulated user interaction to ensure that the UI is in the intermediate state you expect. Without calling it before the interaction, you can't be certain that something actually changed as a consequence of the simulated interaction.

akdetrick · 2017-01-06T15:54:40Z

src/Popover.jsx

+						role='menu'
+						aria-hidden={!isActive}
+					>
+						{


does anyone have a preference for breaking out option rendering into a separate function for readability?

i do

edit: might as well break out the whole optionsList so that you get a complete <ul> from the separate function - it could even be a standalone component, although in this case I'm not sure it would make sense to make it into a separate module

I'll go with renderOptionList as an instance method. Keeping Popover a single, self-contained component feels right.

mmcgahan · 2017-01-06T17:48:54Z

src/Popover.jsx

+		// This zero-length timeout ensures the browser will return the
+		// actual focused element instead of `<body>`
+		window.setTimeout(() => {
+			const focusedOptionClass = document.activeElement.parentNode.getAttribute('class');


use classList to make this easier

mmcgahan · 2017-01-06T17:49:35Z

src/Popover.jsx

+		const targetIndex = this.state.selectedIndex + delta;
+		const optionsLength = this.props.options.length;
+
+		if (targetIndex >= 0 && targetIndex <= optionsLength) {


targetIndex must be less than, not equal to, optionsLength

mmcgahan · 2017-01-06T17:50:39Z

src/Popover.jsx

+			) {
+				this.selectedItemEl.props.onClick(e);
+				break;
+			} else {


no need for else if you have a break in the block above, but it might be more straightforward to keep the else and put the break; outside of the if/else block

nice; this felt a little messy. The line breaks made me feel better about keeping the expression in the if more readable, but is there a better way?

mmcgahan · 2017-01-06T17:56:39Z

src/Popover.jsx

+
+	render() {
+		const isActive = this.state.isActive,
+			{


given that this set of const assignments is so long, I find it more readable to declare const at the beginning of each assignment rather than comma-separating the assignments. In theory, multiple const make for cleaner diffs in refactoring as well.

I was wondering about this; sounds good to me. Chaining seems more reasonable for things like

let just, initializing, some, vars, lol;

mmcgahan · 2017-01-06T18:06:56Z

src/Popover.jsx

+				onKeyUp,
+				onKeyDown,
+				onBlur
+			} = this;


Nitpick, but destructuring instance properties into new consts feels like an over-abstraction that makes usage more ambiguous - leaving them as this.x makes it clear that each method is an instance method rather than a standalone function defined somewhere, with the associated expectation of context binding.

Makes sense; I can see where it could be an annoyance for someone reading this later on

mmcgahan · 2017-01-06T20:44:47Z

src/Popover.jsx

+			) {
+				this.selectedItemEl.props.onClick(e);
+				break;
+			} else {


I think this is how I'd write it, to completely separate the 'open' logic from the 'fire onClick' logic:

if (!this.state.isActive) { this.openMenu(); return; } if (this.selectedItemEl && this.selectedItemEl.props.onClick) { this.selectedItemEl.props.onClick(e); return; }

However, it does feel a little odd for this parent component to be responsible for firing the selectedItem's onClick. I wonder if you could inject an onKeyDown into each option that takes care of that functionality? It's still a little 'controlling', but at least the option fires its own action handlers. That might be over-engineering, though.

issues in the first round of reviews addressed

…-components into WC-7_popover-component * 'WC-7_popover-component' of github.com:meetup/meetup-web-components: (39 commits) clean up focus management fix tests for updated popover structure create dedicated renderer for option items rm unused icons from popover story rm unused components fire click when enter is pressed on selected option add story to demonstrate component flexibility treat user-provided `options` prop elements as the 'menuitem' role style fixes organize classNames working example add prop passed components to render stub popover as a single self-contained component assert component state for each simulated event in popover tests clean up popover tests use state and refs instead of DOM apporach to managing selected items fix bindAll export event handler naming convention accept string or obj for `to` prop of menu item manage selected index with state ...

nlaz · 2017-01-10T17:07:57Z

Made sort of an opinionated change. Refactored options to menuItems because menuItems is more explicit.

mmcgahan · 2017-01-10T19:35:09Z

src/popover.story.jsx

+				menuItems={[
+					<span onClick={logSelection}>First option</span>,
+					<span to='somepath/' onClick={logSelection}>Second option</span>,
+					<span to='somepath/' onClick={logSelection}>Third option</span>,


this to would probably throw an error on a JSX span? 'unsupported property' or something?

…-components into WC-7_popover-component

nlaz · 2017-01-11T00:21:58Z

src/Popover.jsx

+						React.cloneElement(menuItem,
+							{
+								tabIndex: '-1',
+								className: 'popover-menu-option-target',


Setting the className like this overrides the elements className. You can keep the className passed into the prop with:

className: `popover-menu-option-target ${menuItem.props.className}`

However, I'm curious if we want to prevent that by design. Should we allow users of the Popover component set the className of elements within menu? I'm leaning towards allowing it since it will be useful with SQ2 classes.

@nlaz and I talked about this on Slack, and decided to gracefully incorporate menuItem classes since MWC should try to stay as adaptable as possible.

coveralls · 2017-01-11T01:15:34Z

Coverage decreased (-0.2%) to 74.146% when pulling 63cca90 on WC-7_popover-component into b7132c1 on master.

coveralls · 2017-01-11T16:21:57Z

Coverage increased (+5.7%) to 80.097% when pulling 98dfbeb on WC-7_popover-component into b7132c1 on master.

akdetrick added 14 commits December 19, 2016 23:47

stub popover from scratch

6842939

popover trigger and menu components

afbef90

popover trigger and menu components

33c03b2

add menu item; add propTypes to child components

a83857a

set blur and active states for trigger

cb51e97

add arrow nav to popover menu

8660f18

keyup handler on menu items

b3a5d37

fix menu item rendering

78ec367

update story

1e6a689

make menu items focusable

56158c5

Merge branch 'master' into popover_component

9065e0b

fix hover vs focus issues

1522952

try to add onclick handler to menu items"

8567950

convert popover options to links

8b337db

akdetrick added the Prelim label Dec 28, 2016

akdetrick added 2 commits December 28, 2016 15:56

clena up css classes

6c2b23f

add tests

e8f26ed

mmcgahan reviewed Dec 28, 2016

View reviewed changes

mmcgahan previously requested changes Dec 28, 2016

View reviewed changes

akdetrick added 7 commits December 28, 2016 18:44

fix tests

e290318

fix css; add bindAll util

3cba4f2

fix css; add bindAll util

fc0735b

fix tag close style; add propTypes

889b5f8

manage selected index with state

4c8e52f

accept string or obj for to prop of menu item

5b07868

event handler naming convention

24f2ed2

mmcgahan reviewed Dec 29, 2016

View reviewed changes

akdetrick added 2 commits December 30, 2016 13:50

fix bindAll export

410b42d

use state and refs instead of DOM apporach to managing selected items

d245f6f

rm unused icons from popover story

28555a2

akdetrick commented Jan 6, 2017

View reviewed changes

mmcgahan reviewed Jan 6, 2017

View reviewed changes

akdetrick added 2 commits January 6, 2017 18:44

create dedicated renderer for option items

03f13e5

fix tests for updated popover structure

dce2a0b

akdetrick removed the Prelim label Jan 9, 2017

clean up focus management

149a9b6

Lori Hutchek and others added 3 commits January 9, 2017 16:59

adding fall through break

9254acc

fix spacing

f34fd38

Niko Lazaris added 2 commits January 10, 2017 12:08

refactor options to menuitems

32b0fb2

change keyup press to key down press

031e441

mmcgahan reviewed Jan 10, 2017

View reviewed changes

Lori Hutchek and others added 6 commits January 10, 2017 14:54

refactoring selected state setting

9522e4c

cr: wip of getting tests working again

348bb37

fixing tests

c5dfecf

change popover story to allow className and remove prop

d2f4576

clone includes passed in classNames

a6ebb07

Merge branch 'WC-7_popover-component' of github.com:meetup/meetup-web…

a84657f

…-components into WC-7_popover-component

nlaz approved these changes Jan 11, 2017

View reviewed changes

Niko Lazaris added 2 commits January 10, 2017 19:46

fix comma

ee01816

add more unit tests

63cca90

making tests a bit more robust

98dfbeb

eilinora merged commit d6275df into master Jan 11, 2017

chenrui333 deleted the WC-7_popover-component branch September 16, 2019 22:05


		TestUtils.Simulate.click(triggerEl);
		TestUtils.Simulate.keyUp(defaultSelected, {key: 'ArrowDown'});

Wc 7 popover component #30

Wc 7 popover component #30

Uh oh!

Conversation

akdetrick commented Dec 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmcgahan commented Dec 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akdetrick Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmcgahan Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlaz commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 11, 2017

Uh oh!

coveralls commented Jan 11, 2017

Uh oh!

Uh oh!

akdetrick Jan 6, 2017 •

edited

Loading

mmcgahan Jan 6, 2017 •

edited

Loading