Fixes #796 - Add keyboard functionality for clay-dropdown #1282

bryceosterhaus · 2018-11-01T21:10:37Z

I feel like I may have gone the wrong direction on this, but the functionality of groupings really through a wrench in the way the functionality has to work.

matuzalemsteles · 2018-11-08T15:03:29Z

Just started reviewing :)

Sent from GH.

matuzalemsteles · 2018-11-08T15:10:23Z

hey @bryceosterhaus, maybe I think I'd opt to use the focus when navigating, so the scroll can follow the navigation. what do you think?

I'm still taking a look at the implementation.

bryceosterhaus · 2018-11-08T16:33:04Z

@matuzalemsteles oh yeah thats a good idea. We can use both the "active" state and focus. I'll send a follow up commit today using focus.

matuzalemsteles · 2018-11-08T19:33:35Z

packages/clay-dropdown/src/ClayDropdownBase.js

+				const item = data[j];
+
+				if (item.active) {
+					item.checked = !item.checked;


hey @bryceosterhaus, I do not know if it is very ideal to control the checked state inside the component, once it is used with incremental components this state can be lost when some re-rendering happens.

Hmm, not sure what you mean here by "once it is used with incremental components"?

Shouldn't the DOM reflect the state of our component? Otherwise we will be managing state in two different places, both the in the DOM and in the state of the component. Our state already manages active, disabled, and inputValue of each item.

I may not be totally familiar with our patterns for storing state though. I am used to a more reactive way where the DOM is a reflection of our state.

Maybe I was not so clear with that 😅, but when I say incremental components, they are components of Metal.

You're right about this... But in the case of checked is that we're manipulating the state with an action inside our component. This way the state can be lost when a new rendering occurs in the application and thus the item can lose the checked. The Parent component that is calling ClayDropdown will be in a different state from what we are manipulating in the items that are checked, for example. Let me know if I'm being clear on this.

When the item is clicked (_handleItemClick) we propagate this action so that the developer decides what to do with this information, we should do the same when we manipulate item checked so that the developer can do some action and update its state and also to keep the consistency of events.

Oh! I see what you mean. That totally makes sense. I should've looked at _handleItemClick more in depth to see that it was propagating to the parent.

Thanks for the clarification!

matuzalemsteles · 2018-11-08T19:42:34Z

packages/clay-dropdown/src/ClayDropdownBase.js

+	 * @return {Array}
+	 */
+	_flattenItems() {
+		return this.items.map(item => (item.items ? item.items : [item]));


It looks like we're doing the same at https://github.com/liferay/clay/pull/1282/files#diff-1dace496b1f2a92f6bf7984f5a1949a1R112. I'm not sure if it was the same intention, I did not try to test it.

My implementation is slightly different. I might be able to reconcile the two though.

matuzalemsteles · 2018-11-08T20:35:34Z

packages/clay-dropdown/src/ClayDropdownBase.js

@@ -260,6 +434,12 @@ class ClayDropdownBase extends ClayComponent {
 					true
 				),
 				dom.on(document, 'keyup', this._handleKeyup.bind(this), true),
+				dom.on(
+					document,


Maybe we can just attach the event to this.element. what do you think?

matuzalemsteles · 2018-11-08T20:43:07Z

packages/clay-dropdown/src/ClayDropdownBase.js

+	 * @private
+	 */
+	_handleCheckedToggle() {
+		const flattenedItems = this._flattenItems();


hey @bryceosterhaus what do you think of adding a memoize here, decreases the cost of flattening the array of items every time the function is called during user interaction, we just have to make sure to clean it whenever the array of items is changed (we can guarantee this by passing this.items as parameter).

bryceosterhaus · 2018-11-09T00:51:09Z

@matuzalemsteles just updated the PR!

A few things...

I didn't memoize flatten because the argument is an array and regardless the memoize function would need to deeply check the objects in that array so I don't think we would improve any performance.
The event listener doesn't work properly if it is on this.element because the menu renders into a portal, so the even doesn't propagate to this.element. Keeping it on the document seemed like the simplest approach at this point.
I added tabindex to each list item so that when focus() is called, the item will scroll into view.

bryceosterhaus · 2018-11-09T00:54:03Z

@matuzalemsteles also, do you know if there is any sort of command to update all of the isomorphic soy snapshots? Or do I just have to update those all by hand?

matuzalemsteles · 2018-11-09T02:06:50Z

hey @bryceosterhaus, no problems on the case of using memoize, I had forgotten to take into account that we always changed the array items with checked, sorry for that 😅.

The event listener doesn't work properly if it is on this.element because the menu renders into a portal, so the even doesn't propagate to this.element. Keeping it on the document seemed like the simplest approach at this point.

About using this.element is weird, it seems to work fine for me when I try.

matuzalemsteles also, do you know if there is any sort of command to update all of the isomorphic soy snapshots? Or do I just have to update those all by hand?

You can try to go to the clay-isomorphic folder and run at the command line ./gradlew :regenerateFixturesExpected

bryceosterhaus · 2018-11-09T16:43:05Z

@matuzalemsteles the issue for this.element occurs if you use the keyboard to go down a few items. The focus is added to the list item and then the event does not propagate up to this.element because of it being a portal.

matuzalemsteles · 2018-11-09T17:07:04Z

Just started reviewing :)

Sent from GH.

matuzalemsteles · 2018-11-09T17:50:55Z

packages/clay-dropdown/src/ClayDropdownBase.js

+			data: {
+				item: flatten(this.items).find(item => item.active),
+			},
+			name: 'itemSelected',


hey @bryceosterhaus, can you propagate this event to the other components as well?

ClayActionsDropdown

ClayCreationMenuDropdown

ClayDropdown

How does this actually work? I'm a bit confused how these components inherit or extend from DropDownBase

hey @bryceosterhaus, Oh yeah, ClayDropdown follows Lexicon standards as stated in this document, so we have some Dropdown collections here, in the cases above. That way we ensure that people follow the correct Lexicon standards.

We have ClayDropdownBase to be used as a more flexible and somewhat generic component that helps us to reuse it in other parts of Clay components if necessary. We really do not recommend that people use ClayDropdownBase (Maybe use to create other cases...), but rather these others.

We call ClayDropdownBase in the above variations, restricting some APIs and especially the label API.

Hope I have been clear 😅, please let me know if you have any further questions about this.

Oh yeah that makes sense. I just pushed up another commit to propagate the event

matuzalemsteles · 2018-11-09T17:57:48Z

packages/clay-dropdown/src/ClayDropdownBase.js

+			reverse
+		);
+
+		const totalItems = items.flat(2);


I'm just a bit worried about using flat here, it's not supported in some browsers yet and as we support IE 11 for now 🙂... maybe we need a polyfill in Portal, I'm not sure we already have it there. @carloslancha, @julien or @jbalsas do you know anything about this?

Oh good catch. I didn't mean to leave that in there.

matuzalemsteles · 2018-11-13T17:48:49Z

Just started reviewing :)

Sent from GH.

matuzalemsteles

hey @bryceosterhaus this looks good to me!

matuzalemsteles · 2018-11-19T18:21:36Z

hey @bryceosterhaus, Could you organize the commits so we can go ahead with this? @carloslancha do you have any other look on this?

bryceosterhaus · 2018-11-19T20:14:29Z

@matuzalemsteles updated w/ commits squashed

matuzalemsteles · 2018-11-20T17:03:42Z

Thanks @bryceosterhaus! I'm going to merge this and if anyone else has any insight into it comment here or open a issue so we can continue arguing.

matuzalemsteles reviewed Nov 8, 2018

View reviewed changes

bryceosterhaus force-pushed the dropdownImprovements branch from 6ab5409 to 0c1ae73 Compare November 9, 2018 00:47

matuzalemsteles reviewed Nov 9, 2018

View reviewed changes

bryceosterhaus force-pushed the dropdownImprovements branch 2 times, most recently from 4a425cd to 3b8bb07 Compare November 12, 2018 23:36

matuzalemsteles approved these changes Nov 13, 2018

View reviewed changes

Fixes liferay#796 - Add keyboard functionality for dropdowns

85d4d7e

bryceosterhaus force-pushed the dropdownImprovements branch from 3b8bb07 to 85d4d7e Compare November 19, 2018 20:13

matuzalemsteles merged commit 64fa3f3 into liferay:develop Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #796 - Add keyboard functionality for clay-dropdown #1282

Fixes #796 - Add keyboard functionality for clay-dropdown #1282

bryceosterhaus commented Nov 1, 2018

matuzalemsteles commented Nov 8, 2018

matuzalemsteles commented Nov 8, 2018

bryceosterhaus commented Nov 8, 2018

matuzalemsteles Nov 8, 2018

bryceosterhaus Nov 8, 2018

matuzalemsteles Nov 8, 2018

bryceosterhaus Nov 8, 2018

matuzalemsteles Nov 8, 2018

bryceosterhaus Nov 8, 2018

matuzalemsteles Nov 8, 2018

matuzalemsteles Nov 8, 2018

bryceosterhaus commented Nov 9, 2018 •

edited

Loading

bryceosterhaus commented Nov 9, 2018

matuzalemsteles commented Nov 9, 2018

bryceosterhaus commented Nov 9, 2018

matuzalemsteles commented Nov 9, 2018

matuzalemsteles Nov 9, 2018

bryceosterhaus Nov 9, 2018

matuzalemsteles Nov 10, 2018

bryceosterhaus Nov 12, 2018

matuzalemsteles Nov 9, 2018

bryceosterhaus Nov 10, 2018

matuzalemsteles commented Nov 13, 2018

matuzalemsteles left a comment

matuzalemsteles commented Nov 19, 2018

bryceosterhaus commented Nov 19, 2018

matuzalemsteles commented Nov 20, 2018

Fixes #796 - Add keyboard functionality for clay-dropdown #1282

Fixes #796 - Add keyboard functionality for clay-dropdown #1282

Conversation

bryceosterhaus commented Nov 1, 2018

matuzalemsteles commented Nov 8, 2018

matuzalemsteles commented Nov 8, 2018

bryceosterhaus commented Nov 8, 2018

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

bryceosterhaus commented Nov 9, 2018 • edited Loading

bryceosterhaus commented Nov 9, 2018

matuzalemsteles commented Nov 9, 2018

bryceosterhaus commented Nov 9, 2018

matuzalemsteles commented Nov 9, 2018

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

matuzalemsteles commented Nov 13, 2018

matuzalemsteles left a comment

Choose a reason for hiding this comment

matuzalemsteles commented Nov 19, 2018

bryceosterhaus commented Nov 19, 2018

matuzalemsteles commented Nov 20, 2018

bryceosterhaus commented Nov 9, 2018 •

edited

Loading