feat(List): add horizontal prop #1721

mnajdova · 2019-07-26T09:43:08Z

Added horizontal prop to the List component. This prop is considered in the keyboard navigation of the selectable List.

-changed list behavior

codecov · 2019-07-26T10:31:25Z

Codecov Report

Merging #1721 into master will increase coverage by 0.07%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
+ Coverage   70.98%   71.05%   +0.07%     
==========================================
  Files         860      860              
  Lines        7156     7170      +14     
  Branches     2058     2082      +24     
==========================================
+ Hits         5080     5095      +15     
+ Misses       2070     2069       -1     
  Partials        6        6

Impacted Files	Coverage Δ
packages/react/src/components/List/List.tsx	`59.25% <ø> (ø)`	⬆️
...c/lib/accessibility/Behaviors/List/listBehavior.ts	`100% <ø> (ø)`	⬆️
...act/src/themes/teams/components/List/listStyles.ts	`20% <0%> (+3.33%)`	⬆️
...ages/react/test/specs/behaviors/testDefinitions.ts	`93.41% <100%> (+0.32%)`	⬆️
...ssibility/Behaviors/List/selectableListBehavior.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06cd244...9c7a8aa. Read the comment docs.

lucivpav · 2019-07-26T12:50:55Z

Since the default value of vertical is true, wouldn't it be more user-friendly, if we would specify horizontal instead? E.g. <List horizontal> or <List horizontal={true}>?

mnajdova · 2019-07-26T14:48:17Z

@lucivpav that's a good point, but I chose the vertical only because we have that as a term in other components too. I think it will be easier for users to just learn that one prop, rather than remember where they should provide the vertical, and where the horizontal prop. Is this strong enough reason for you? :)

lucivpav · 2019-07-26T14:50:49Z

@lucivpav that's a good point, but I chose the vertical only because we have that as a term in other components too. I think it will be easier for users to just learn that one prop, rather than remember where they should provide the vertical, and where the horizontal prop. Is this strong enough reason for you? :)

Yes, it makes very good sense now :)

kolaps33 · 2019-07-29T10:00:19Z

packages/react/src/lib/accessibility/Behaviors/List/selectableListBehavior.ts

+ * Triggers 'moveNext' action with 'ArrowDown' on 'root', when orientation is vertical.
+ * Triggers 'moveNext' action with 'ArrowRight' on 'root', when orientation is horizontal.
+ * Triggers 'movePrevious' action with 'ArrowUp' on 'root', when orientation is vertical.
+ * Triggers 'movePrevious' action with 'ArrowLeft' on 'root', when orientation is horizontal.
 * Triggers 'moveFirst' action with 'Home' on 'root'.
 * Triggers 'moveLast' action with 'End' on 'root'.
 */


Please add "aria-orientation" attribute(on the root element, under line 19), based on the aria:
https://www.w3.org/TR/wai-aria-practices/#Listbox
If options are arranged horizontally, the element with role listbox has aria-orientation set to horizontal. The default value of aria-orientation for listbox is vertical.

Thank you :)

Added, please take a look and propose how we can test it :\

levithomason · 2019-07-29T17:01:14Z

docs/src/examples/components/List/Variations/ListExampleHorizontal.shorthand.tsx

+]
+
+const ListExampleSelectable = () => (
+  <List selectable defaultSelectedIndex={0} items={items} vertical={false} />


Propose we use horizontal instead. This is because the default list is expected to be vertical and the descriptive alternative is one that is horizontal.

This is similar to why there is an HTML attribute for disabled and not enabled="false".

Okay, my reasoning was that we already have the vertical prop, so users will have to think about before applying vertical/horizontal on different components, based on how they appear by default...

levithomason · 2019-07-29T17:03:50Z

packages/react/src/themes/teams/components/List/listStyles.ts

-  },
+  root: ({ props: p }): ICSSInJSStyle => ({
+    ...(p.debug && debugRoot()),
+    display: p.vertical ? 'block' : 'flex',


What's the reasoning on horizontal lists being block level elements vs inline elements (inline-flex)? What's use cases are we targeting?

No strong reasoning for this, I was just following the paradigm we had before. inline-flex works as expected as well, and may be even better to the usecases we have, something like this:

Will change it

-updated behavior description

# Conflicts: # CHANGELOG.md

manajdov added 2 commits July 26, 2019 11:35

-added vertical prop on the list

709cd3b

-changed list behavior

-added horizontal example

7d0a4ba

mnajdova requested review from dvdzkwsk, kuzhelov, layershifter, levithomason and miroslavstastny as code owners July 26, 2019 09:43

-updated changelog

278401e

vercel bot deployed to staging July 26, 2019 09:45 View deployment

-updated description on the selectable list behavior

4c24bd0

vercel bot deployed to staging July 26, 2019 10:14 View deployment

mnajdova added the 🚀 ready for review label Jul 26, 2019

kolaps33 reviewed Jul 29, 2019

View reviewed changes

-added horizontal orientation aria attributes

e04d17a

vercel bot deployed to staging July 29, 2019 15:27 View deployment

levithomason reviewed Jul 29, 2019

View reviewed changes

-changed flex to inline-flex

597b02e

vercel bot deployed to staging July 30, 2019 09:22 View deployment

-changed example for horizontal list

d6774b7

vercel bot deployed to staging July 30, 2019 09:31 View deployment

-renamed vertical to horizontal

62651df

-updated behavior description

mnajdova changed the title ~~feat(List): add vertical prop~~ feat(List): add horizontal prop Jul 31, 2019

vercel bot temporarily deployed to staging July 31, 2019 08:36 Inactive

Merge branch 'master' into feat/list-vertical-prop

9c7a8aa

# Conflicts: # CHANGELOG.md

vercel bot deployed to staging July 31, 2019 08:41 View deployment

layershifter approved these changes Jul 31, 2019

View reviewed changes

mnajdova merged commit b06f6b3 into master Jul 31, 2019

delete-merged-branch bot deleted the feat/list-vertical-prop branch July 31, 2019 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(List): add horizontal prop #1721

feat(List): add horizontal prop #1721

mnajdova commented Jul 26, 2019 •

edited

codecov bot commented Jul 26, 2019 •

edited

lucivpav commented Jul 26, 2019

mnajdova commented Jul 26, 2019

lucivpav commented Jul 26, 2019

kolaps33 Jul 29, 2019

mnajdova Jul 30, 2019

levithomason Jul 29, 2019

mnajdova Jul 30, 2019

levithomason Jul 29, 2019

mnajdova Jul 30, 2019

feat(List): add horizontal prop #1721

feat(List): add horizontal prop #1721

Conversation

mnajdova commented Jul 26, 2019 • edited

codecov bot commented Jul 26, 2019 • edited

Codecov Report

lucivpav commented Jul 26, 2019

mnajdova commented Jul 26, 2019

lucivpav commented Jul 26, 2019

kolaps33 Jul 29, 2019

Choose a reason for hiding this comment

mnajdova Jul 30, 2019

Choose a reason for hiding this comment

levithomason Jul 29, 2019

Choose a reason for hiding this comment

mnajdova Jul 30, 2019

Choose a reason for hiding this comment

levithomason Jul 29, 2019

Choose a reason for hiding this comment

mnajdova Jul 30, 2019

Choose a reason for hiding this comment

mnajdova commented Jul 26, 2019 •

edited

codecov bot commented Jul 26, 2019 •

edited