-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
-changed list behavior
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Since the default value of |
@lucivpav that's a good point, but I chose the |
Yes, it makes very good sense now :) |
* 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'. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, please take a look and propose how we can test it :\
] | ||
|
||
const ListExampleSelectable = () => ( | ||
<List selectable defaultSelectedIndex={0} items={items} vertical={false} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
}, | ||
root: ({ props: p }): ICSSInJSStyle => ({ | ||
...(p.debug && debugRoot()), | ||
display: p.vertical ? 'block' : 'flex', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning on horizontal lists being block level elements vs inline elements (inline-flex
)? What's use cases are we targeting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-updated behavior description
# Conflicts: # CHANGELOG.md
Added horizontal prop to the
List
component. This prop is considered in the keyboard navigation of the selectable List.