FAI-529: Score Cards: Data Dictionary: Remove duplication of delete icons #619
Conversation
...ml-editor/src/editor/components/MiningSchema/MiningSchemaAddFields/MiningSchemaAddFields.tsx
Show resolved
Hide resolved
...ges/pmml-editor/src/editor/components/MiningSchema/MiningSchemaFields/MiningSchemaFields.tsx
Show resolved
Hide resolved
|
Reminder: Our CI doesn't run on draft PRs. |
...ges/pmml-editor/src/editor/components/MiningSchema/MiningSchemaFields/MiningSchemaFields.tsx
Show resolved
Hide resolved
...tor/src/editor/components/DataDictionary/DataDictionaryContainer/DataDictionaryContainer.tsx
Outdated
Show resolved
Hide resolved
...tor/src/editor/components/DataDictionary/DataDictionaryContainer/DataDictionaryContainer.tsx
Show resolved
Hide resolved
...tor/src/editor/components/DataDictionary/DataDictionaryContainer/DataDictionaryContainer.tsx
Outdated
Show resolved
Hide resolved
packages/pmml-editor/src/editor/components/DataDictionary/DataTypeItem/DataTypeItem.tsx
Show resolved
Hide resolved
| onDelete(e); | ||
| onClick={(e) => handleDelete(e, "mouse")} | ||
| onKeyDown={(event) => { | ||
| if (event.key === "Enter") { |
There was a problem hiding this comment.
@manstis I understand the idea: when I have focused this button and pressed enter then I delete item. In my point of view is better just "click" on button. It is very difficult to select this button to perform "Keydown event" - "enter". When I click on the delete button I delete it. I need to perform "left click" + "enter" or press severel time "tab" (focus attribute, focus predicate, focus delete predicate, focus detele attribute) and then "enter" to delete. Do you have some hint how use keyboard easily?
There was a problem hiding this comment.
@sjamboro Many users will no doubt "click" on the icon with the mouse pointer. However to maximise accessiblity for users with disabilities we need to also provide support for screen readers and keyboard operation. I agree it does make writing an integration test an interesting exercise however I do something similar in my unit tests.
See here for an example test. I have to first focus the row then re-render the component (so the delete icon is visible) and then find the delete icon and emulate a keyboard event. The focus/keyboard event should be possible with an integration test but you would not have to re-render the component as it'd be running "live" under Cypress.
There was a problem hiding this comment.
@manstis you mention important thing about the "delete" button visibility. We have to focus the row element. Is touchscreen supported or not? Do you have experience how it looks on touchscreen? Based on this side I am afraid that the user with touchscreen cannot delete any row. The "delete" icon is visible only if the row is focused or hovered.
There was a problem hiding this comment.
See https://issues.redhat.com/browse/FAI-577. This comment does highlight a flaw in this whole approach.
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onEdit(); | ||
| handleEdit(e); |
There was a problem hiding this comment.
@manstis should be possible to press "delete" to perform handleDelete(event, "keyboard") and focus another item on the list?
There was a problem hiding this comment.
No @sjamboro it should not be possible to press delete in this PR. Pressing enter when a HTML element has the focus is the standard way to replicate "click events" with the keyboard (see how any other button behaves on a web site). We could raise a JIRA for UX to comment on keyboard shortcuts of which delete deleting a row would be a consideration.
There was a problem hiding this comment.
@manstis I do not if there is missunderstood. I have no doub about pressing enter instead of click. That is correct. I mean allow enter and delete key. enter to edit row and delete to delete row. I understand if you need the new JIRA. I was not sure about scope of this JIRA.
There was a problem hiding this comment.
See https://issues.redhat.com/browse/FAI-577. We could add support for [enter] putting a row into "edit mode".
| expect(container).toMatchSnapshot(); | ||
|
|
||
| expect(getByTestId("attribute-n0")).not.toBeUndefined(); | ||
| expect(getByTestId("attribute-n1")).not.toBeUndefined(); |
There was a problem hiding this comment.
@manstis can you assert number of row or "attribute-n2" to be null?
|
@manstis I am going to check that the delete button is visible and accesible via cypress test. I want to create new JIRAs (DD, Mining, Outpust, Characteristics). Your tests are ok. It would be nice to check the content of the row ("field1" and "field2") and missing icon without hover or focus. |
|
@tiagobento @ederign This PR is ready for your eyes now 👍 |
…cons (apache#619) * FAI-529: Score Cards: Data Dictionary: Remove duplication of delete icons * DataDictionary unit tests. * OutputFieldsTable unit tests. * Update DataDictionaryContainer snapshots. * MiningSchema unit tests. * CharacteristicsTable unit tests. * AttributesTable unit tests. * Fix integration tests. Fix 'Add Output' functionality. * Updates following discussion with kelvah. Co-authored-by: Michael Anstis <manstis@redhat.com>
See https://issues.redhat.com/browse/FAI-529
@sjamboro What type of tests would you like to see for what is largely a CSS related change?
Characteristics are covered by the
predicate.tsintegration test.I'll see if I can put together some form of unit test.