Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ENH Improve keyboard support #200

Merged
merged 1 commit into from
Feb 12, 2024
Merged

ENH Improve keyboard support #200

merged 1 commit into from
Feb 12, 2024

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Feb 1, 2024 •
edited
Loading

Issue #99

Does the following:

  • Tab through links and press enter (or space) to open the modal, even in read-only
  • If closing or submitting the modal using the keyboard, focus should be returned to the button which would open the modal for that link (or to the "add link" button for new link modal)
  • Reorder links with the keyboard - tab to the link handle, press enter/space then press up/down or left/right, then press enter/space to accept
  • Focuses on the first input when opening the link modal with keyboard

Jest testing proved extremely hard so I haven't done much of it in this PR. I switched to using React Testing Library userEvent though it still didn't behave like a browser, for instance:

  • keyboard sorting isn't possible because jsdom doesn't have a layout engine so cannot calculated new coords
  • pressing enter/space on a react strap dropdown item didn't trigger the onclick handler which is triggered when doing the same in an actual browser

New issue for behat keyboard sorting:

@emteknetnz emteknetnz mentioned this pull request Feb 1, 2024
Closed
@emteknetnz emteknetnz force-pushed the pulls/4/keyboard branch 4 times, most recently from 1e145aa to 1de49b9 Compare February 1, 2024 23:08
emteknetnz
emteknetnz commented Feb 1, 2024
View reviewed changes
client/src/components/LinkField/LinkField.js
@@ -42,7 +49,6 @@ const section = 'SilverStripe\\LinkField\\Controllers\\LinkFieldController';
const LinkField = ({
value = null,
onChange,
onNonPublishedVersionedState,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated refactoring to remove a non-existant prop

emteknetnz
emteknetnz commented Feb 1, 2024
View reviewed changes
client/src/components/LinkField/LinkField.js
import { arrayMove, SortableContext, verticalListSortingStrategy } from '@dnd-kit/sortable';
import { restrictToVerticalAxis, restrictToParentElement } from '@dnd-kit/modifiers';
import { injectGraphql } from 'lib/Injector';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated refactoring to remove an unused import

emteknetnz
emteknetnz commented Feb 1, 2024
View reviewed changes
client/src/components/LinkPicker/LinkPicker.scss
left: 5px;
position: absolute;
z-index: 100;

&:hover {
cursor: grab;
}

.font-icon-drag-handle {
opacity: 0%;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using opacity instead of display so that it can be tabbed to

@emteknetnz emteknetnz force-pushed the pulls/4/keyboard branch 2 times, most recently from 96e9f0e to d08a599 Compare February 1, 2024 23:28
emteknetnz
emteknetnz commented Feb 1, 2024
View reviewed changes
client/src/components/LinkPicker/LinkPickerTitle.js
@@ -108,15 +143,7 @@ const LinkPickerTitle = ({
role="button"
tabIndex="0"
className="link-picker__delete btn btn-link"
onKeyDown={(evt) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated refactoring, moved to handleDeleteKeyDown()

emteknetnz
emteknetnz commented Feb 1, 2024
View reviewed changes
client/src/components/LinkPicker/LinkPickerTitle.js
}
};

const handleDeleteKeyDown = (event) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated refactoring, was previously an inline method in render() for onKeyDown

@emteknetnz emteknetnz force-pushed the pulls/4/keyboard branch 7 times, most recently from 17cc97b to 66e8f94 Compare February 7, 2024 05:11
@emteknetnz emteknetnz marked this pull request as ready for review February 7, 2024 05:13
@emteknetnz emteknetnz mentioned this pull request Feb 7, 2024
Closed
sabina-talipova
sabina-talipova suggested changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, works perfect. Just tiny possible improvements.

client/src/components/LinkPicker/LinkPicker.scss Show resolved Hide resolved
client/src/components/LinkPicker/LinkPickerTitle.js Outdated
Comment on lines 129 to 143
{ (isMulti && !readonly && !disabled) && <div className="link-picker__drag-handle">
<i
className="font-icon-drag-handle"
tabIndex="0"
role="button"
aria-pressed="false"
aria-controls={idAttr}
onKeyDown={handleIconKeyDown}
></i>
</div> }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ (isMulti && !readonly && !disabled) && <div className="link-picker__drag-handle">
<i
className="font-icon-drag-handle"
tabIndex="0"
role="button"
aria-pressed="false"
aria-controls={idAttr}
onKeyDown={handleIconKeyDown}
></i>
</div> }
{ (isMulti && !readonly && !disabled) && <div className="link-picker__drag-handle"
tabIndex="0"
role="button"
aria-pressed="false"
aria-controls={idAttr}
aria-label="Sort Links"
onKeyDown={handleIconKeyDown}
>
<i
className="font-icon-drag-handle"
aria-hidden="true"
focusable="false"
></i>
</div> }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, would be better to use as a button div element instead of icon.
And also, please, add aria-label="Sort Links" since this element doesn't have any content that explains what this button does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

client/src/components/LinkPicker/LinkPickerTitle.js Show resolved Hide resolved
client/src/components/LinkPicker/LinkPickerTitle.js Show resolved Hide resolved
client/src/components/LinkField/LinkField.js Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4/keyboard branch 2 times, most recently from bd7ecb5 to 52c3908 Compare February 11, 2024 23:48
sabina-talipova
sabina-talipova suggested changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emteknetnz, could you please fix this one as well https://github.com/silverstripe/silverstripe-linkfield/pull/200/files#r1483616400

@emteknetnz
Copy link
Member Author

@sabina-talipova Have updated

sabina-talipova
sabina-talipova approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Test locally, works perfect.

@sabina-talipova sabina-talipova merged commit 9340c28 into silverstripe:4 Feb 12, 2024
10 checks passed
@sabina-talipova sabina-talipova deleted the pulls/4/keyboard branch February 12, 2024 19:35
@maxime-rainville maxime-rainville mentioned this pull request Mar 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sabina-talipova sabina-talipova sabina-talipova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emteknetnz @sabina-talipova