-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: row navigation in Table #1231
Conversation
Deploy preview for fundamental-react ready! Built with commit 55c6313 |
src/Table/Table.js
Outdated
const navigatedVertically = (key === keycode.codes.up || key === keycode.codes.down) && row > 0; | ||
|
||
if (navigatedVertically) { | ||
newInstructionsText += `${localizedText.row} ${row} `; |
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.
I think this string needs to have the row number as a variable rather than concatenated. This wouldn't be grammatical in some other languages. Same for column.
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.
Decided to remove the row and column numbers as screen readers still give that information anyway when using the built-in table navigation keys (ctrl+option/alt+arrow on VO and JAWS). And it doesn't seem prudent to try and work in a localization system that supports variables right now.
src/Table/Table.js
Outdated
if (keyboardNavigation !== 'row') { | ||
newInstructionsText += `${localizedText.arrowKeys} `; | ||
} else { | ||
newInstructionsText += `${localizedText.rowArrowKeys} ${localizedText.rowSelection} ${localizedText.rowClick}`; |
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.
Similar comment here about concatenating localized strings.
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.
Decided to combine the row instructions into a single prop to prevent concatenation.
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.
Localized text changes looks good and keyboard handling works well
Description
onSelectRow
onClickRow
richTable
prop with aselection
prop containingonClickRow
,isSelected
, andonSelectRow
propsKnown Issues
:active
state styling interferes with selected row styling when selecting a row with the mouse. Will need to be fixed infundamental-styles
.:active
styling does not trigger on enter key, only on mouse click (https://stackoverflow.com/questions/32467397/how-to-trigger-the-active-pseudoclass-on-keyboard-enter-press-using-only-cs). This is native behavior.