feat: add keyboard controls for widget layout #795
Conversation
| * @param event {Object} The event object | ||
| * @return {boolean} True if proper conditions aren't met | ||
| */ | ||
| $scope.moveWithKeyboard = function(widget, event) { |
There was a problem hiding this comment.
Could something like fluid drag and drop work here? https://wiki.fluidproject.org/display/fluid/Drag+and+Drop+Design+Pattern
Or could this be contributed upstream so that everyone can enjoy an accessible experience?
There was a problem hiding this comment.
Personally I don't agree that sortable widgets is something that should be in the app framework. I don't even think widgets should be in the app framework at all. We included them because on-campus developers using the framework to develop other apps asked us to, and in retrospect I would have fought against doing it.
Hopefully there comes a day when the app framework becomes a collection of API-driven web components. Then this separation of concerns won't be an issue. 🤷♂️
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't agree that sortable widgets is something that should be in the app framework.
I agree 👍
Hence the suggestion to contribute it upstream at Angular drag and drop lists.
Thanks @apetro for providing the link to the issue. 🙇
| * @return {number} Index of the desired item or -1 | ||
| */ | ||
| var findLayoutIndex = function(array, attribute, value) { | ||
| for (var i = 0; i < array.length; i+= 1) { |
There was a problem hiding this comment.
| @@ -239,7 +329,7 @@ define(['angular', 'jquery'], function(angular, $) { | |||
| /** | |||
| * Sets href attribute for 'BASIC' type widget | |||
| * @param portlet | |||
| * @returns {*} | |||
There was a problem hiding this comment.
https://github.com/gajus/eslint-plugin-jsdoc recommends returns. Either return or returns works for me.
Either way we should add that to the lint rules and make it consistent across the app.
There was a problem hiding this comment.
It's in there already and triggers a warning. The instances changed in this PR were fixed automatically by running eslint with the --fix flag.
| * @param event {Object} The event object | ||
| * @return {boolean} True if proper conditions aren't met | ||
| */ | ||
| $scope.moveWithKeyboard = function(widget, event) { |
There was a problem hiding this comment.
How is this different from web/src/main/webapp/my-app/layout/controllers.js line 79-130? are both needed?
There was a problem hiding this comment.
Not necessarily, no. It's duplicated because the two views use different controllers, and refactoring those controllers isn't in the scope of this story. It's something we should do at some point.
| * @param fname {String} The moved widget's fname | ||
| * @param dropIndex {Number} Index where the widget landed | ||
| * @param startIndex {Number} (optional) Index before moving the widget | ||
| * @return {boolean} |
There was a problem hiding this comment.
What are the semantics of the boolean returned? true means what? false means what?
| * Respond to arrow key-presses when focusing a movable list element | ||
| * @param widget {Object} The widget trying to move | ||
| * @param event {Object} The event object | ||
| * @return {boolean} True if proper conditions aren't met |
There was a problem hiding this comment.
What's up with the boolean return? true means didn't do anything? Is this idiomatic? In some Java methods, for instance, true means had an effect and false means didn't have an effect, e.g. Set.add() .
There was a problem hiding this comment.
Agree it's semantically meaningless, just the result of hasty writing. This is something I can clean up!
| var nextIndex = currentIndex + 1; | ||
|
|
||
| // left or up | ||
| if (event.which === 37 || event.which === 38) { |
There was a problem hiding this comment.
Magic constants? Worth abstracting? Can they be referenced from somewhere?
MUMUP-3249: "As a keyboard user I would like to re-order my layout so that I can customize my MyUW experience to my needs."
In this PR:
Related PR: uportal-app-framework#721
Demos
New drag and drop
Keyboard controls
New responsive layout
Contributor License Agreement adherence: