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

support actions on rows #4023

Merged
merged 9 commits into from
Oct 23, 2019
Merged

support actions on rows #4023

merged 9 commits into from
Oct 23, 2019

Conversation

iantrich
Copy link
Member

@iantrich iantrich commented Oct 16, 2019

Continuation of #3837 (Didn't want to deal with rebase when I'm changing the approach as well)

Docs: home-assistant/home-assistant.io#10826

type: entities
entities:
  - entity: sensor.breaking_changes
    hold_action:
      action: url
      url_path: https://www.home-assistant.io

@bramkragten
Copy link
Member

Have you convinced @balloob already about this?

@iantrich
Copy link
Member Author

Have you convinced @balloob already about this?

🙏

@iantrich
Copy link
Member Author

Moved the action handling to the generic-row's name after some discussion with @balloob.
Only issue I currently see is that the input-select row doesn't use the generic-row and only displays the state-badge to the left.

@balloob
Copy link
Member

balloob commented Oct 23, 2019

I am fine with these changes if it does not hurt accessibility. Are we able to use the tab key to select the name and open the more info?

@iantrich
Copy link
Member Author

I am fine with these changes if it does not hurt accessibility. Are we able to use the tab key to select the name and open the more info?

We don't currently support this, the only thing that is selectable on the demo by tab are controls which is still true after my change.

@iantrich
Copy link
Member Author

@balloob while checking tabs, I'm finding lots of holes in our accessibility features. i.e. you can't highlight many elements, can't trigger anything associated with long-press using enter or ha-switch. @bramkragten and I are thinking that 0.102 should be a feature-freeze release with focus on these issues and hacktoberfest cleanup.

With that in mind, I think this should be good to merge for 0.101 beta. Thanks.

@balloob
Copy link
Member

balloob commented Oct 23, 2019

Accessibility is very important. Not supporting that would literally cause us to exclude an already vulnerable part of the people. We've been slowly working on improving accessibility but still have a long way to go. Saying that we currently have a lot of big holes is not a reason to add to it. If we can't mark the element as interactive and allow tabbing to it to allow the user to activate the click handler using a keyboard, we shouldn't merge this. Double click and long press we can ignore, as they won't be part of the default generated UI config.

@iantrich
Copy link
Member Author

@balloob I'm not taking away any accessibility as our current rows are not marked and I think it would be misleading to mark it as interactive now until we fix the click handler.

@balloob
Copy link
Member

balloob commented Oct 23, 2019

I'm down to merge this PR and add this feature, but we should make sure that it won't break our path to making these elements interactive. Because if that happens, we would end up having to remove it again, which pisses people off too. That's why I want to make sure that keyboard nav still works. And if we don't have examples of this working with long press directive, nor are we able to make it work, then the long press directive should not be used.

I think that it can work if we use a <button> or an <a> tag with href attribute + preventDefault() in the click listener.

@iantrich
Copy link
Member Author

That's why I want to make sure that keyboard nav still works

It doesn't work currently, is what I'm saying.

Regardless, I've pushed a commit that allows us to handle enter in long-press elements and made the state-badge (as input-select doesn't have a name in the row) to be tabbable and brings up the tap-action on enter.

@balloob balloob merged commit 39d0522 into home-assistant:dev Oct 23, 2019
@iantrich iantrich deleted the actions-rows branch October 23, 2019 17:34
@@ -138,9 +138,16 @@ class LongPress extends HTMLElement implements LongPress {
window.setTimeout(() => (this.cooldownEnd = false), 100);
};

const handleEnter = (ev: Event) => {
Copy link
Member

Choose a reason for hiding this comment

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

    const handleEnter = (ev: KeyboardEvent) => {

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants