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.

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

[Designer][Accessibility] Click to add elements #4828

Merged
merged 11 commits into from
Sep 29, 2020
Merged

Conversation

golddove
Copy link
Member

@golddove golddove commented Sep 26, 2020

Related Issue

ADO 24096354

Description

Click (or press space/enter) to append an element to bottom of card. This is an intermediate fix, as it does not allow full keyboard replacement for drag/drop (namely, there is no support for reordering elements).

Also, switched keycode constants to use KeyboardEvent.key constants, as the keyCode is long deprecated.

How Verified

Manual verification

Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Sep 26, 2020

Hi @golddove. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

Copy link
Member

@dclaux dclaux 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 overall, but I think I have a problem with doing this on "click". Click is a mouse event; if someone can use the mouse, they are likely to be able to do drag/drop. If they really want to use a shortcut to adding an element at the end of the card, then I think that should be on double click for two reasons:

  • With single click, I think there is too much of a potential for a user that was trying to initiate a drag drop operation to end up generating a single click instead
  • Many (most? all?) the designer experiences that I've used (WinForms, XAML, Delphi, PowerBuilder, Visual Basic) do this on double click

Beyond the above, I think the newly created peer should be automatically selected.

export const KEY_UP = 38;
export const KEY_DOWN = 40;
export const KEY_DELETE = 46;
export const KEY_TAB = "Tab";
Copy link
Member

Choose a reason for hiding this comment

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

These are exported constants. Any code that currently uses them will be broken. We have to decide whether that's OK or not.

On one hand, it is unlikely that this will affect a lot of applications; on the other hand, as a general rule we just don't introduce breaking changes unless they're absolutely necessary.

Given they are not absolutely necessary here, I think we should keep the old constants as is and introduce the new ones. We can use the <name>Key naming convention: export const TabKey = "Tab";

@ghost ghost removed the Needs: Author Feedback label Sep 29, 2020
source/nodejs/adaptivecards-controls/src/constants.ts Outdated Show resolved Hide resolved
static readonly hints = {
dontUseWeightedAndStrecthedColumnsInSameSet: () => "It is not recommended to use weighted and stretched columns in the same ColumnSet, because in such a situation stretched columns will always get the minimum amount of space."
}
} as const;
Copy link
Member

Choose a reason for hiding this comment

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

Not familiar with this - what does it do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Applies readonly compile-time type to the properties in object literals (to prevent accidental object mutation).

In this case, it is equivalent to:

static readonly hints : {
    readonly dontUseWeightedAndStrecthedColumnsInSameSet : () => String
} = {
    dontUseWeightedAndStrecthedColumnsInSameSet: () => ...
}

which would be very unwieldy syntax.

source/nodejs/adaptivecards-designer/src/card-designer.ts Outdated Show resolved Hide resolved
source/nodejs/adaptivecards-designer/src/card-designer.ts Outdated Show resolved Hide resolved
source/nodejs/adaptivecards-designer/src/card-designer.ts Outdated Show resolved Hide resolved
@ghost ghost removed the Needs: Author Feedback label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants