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

Lovelace - Long Press for everything #1848

Merged
merged 5 commits into from
Oct 26, 2018
Merged

Lovelace - Long Press for everything #1848

merged 5 commits into from
Oct 26, 2018

Conversation

thomasloven
Copy link
Contributor

@thomasloven thomasloven commented Oct 24, 2018

A working directive for long-press. Applied to LoveLace glance card.

This might not need to be limited only to lovelace...

I hate naming things. Suggestions for names that make sense are highly appreciated.

I also pushed this to home-assistant:long-press so it's easier for people to test.

Documentation

home-assistant/home-assistant.io#7111

@ghost ghost assigned thomasloven Oct 24, 2018
@ghost ghost added the in progress label Oct 24, 2018
@@ -16,6 +16,7 @@ import { fireEvent } from "../../../common/dom/fire_event.js";
import { HassLocalizeLitMixin } from "../../../mixins/lit-localize-mixin";
import { HomeAssistant } from "../../../types.js";
import { LovelaceCard, LovelaceConfig } from "../types.js";
import { longPress } from "../common/long-press.ts";
Copy link
Member

Choose a reason for hiding this comment

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

Can we make a new folder for directives and name it long-press-directive.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be a directive, or should we think bigger?


const firstSetup = (element.longPress == null);

element.longPress = {
Copy link
Member

Choose a reason for hiding this comment

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

No need to set it as property on the element, you will always get passed the same part, so you can use part.value and part.setValue

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value seems to be reset on each new render, so this doesn't work.

Also, setting it as a element property allows longPressBind to be called directly - so it can be used outside of lit-html templates as well. That's why I export that too.

Copy link
Member

Choose a reason for hiding this comment

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

It is not supposed to be re-set on each render, that's how Lit maintains state. Are you sure you're actually dealing with the same element when value is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding

console.log(part.value.test);
part.setValue({test: "hello"});

to the directive just fills the log with undefined, with more popping up when rerendering (such as by changing window size).

Copy link
Member

Choose a reason for hiding this comment

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

But what about the element? This one: const surfaceNode = part.committer.element as HTMLElement; is it the same?

You can verify by using a weakMap.

// outside directive
const interactionNodes = new WeakMap();
// inside directive
console.log(interactionNodes.get(part.committer.element))
interactionNodes.set(part.committer.element, "yooo")

@balloob
Copy link
Member

balloob commented Oct 24, 2018

amazing.

@thomasloven
Copy link
Contributor Author

I made some changes.

  • The interface is changed.
    Now a hold event is fired to the element when held, and a clicked when clicked.
    Unfortunately, I need to intercept the normal click, so using that messes things up, but this looks a bit more like a native interface.
  • The controlling functions have been bundled together into a custom <long-press> element that's added to the page body.

@thomasloven
Copy link
Contributor Author

1ce7fdc

This shows how long-pressability can be added to elements without lit-html templates.

@thomasloven
Copy link
Contributor Author

Finished up and cleaned up the git history.

Should all this be one PR, or should I make one for the controller and directive, and then separate ones for each implementation?

longpress

@thomasloven thomasloven changed the title WIP - LL Long Press for everything Lovelace - Long Press for everything Oct 25, 2018
@@ -157,7 +163,7 @@ class HuiEntityButtonCard extends hassLocalizeLitMixin(LitElement)
return `hsl(${hue}, 100%, ${100 - sat / 2}%)`;
}

private handleClick() {
private handleClick(hold=false) {
Copy link
Member

Choose a reason for hiding this comment

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

why add a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... I... don't know... It was needed in some case before, but I've since then changed things so it isn't...

private stopAnimation() {
this.ripple.active = false;
this.ripple.disabled = true;
this.style.visibility = "hidden";
Copy link
Member

Choose a reason for hiding this comment

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

should this be visibility or display: none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

display: none feels more right. I'll change it.

@@ -32,7 +33,13 @@ class HuiIconElement extends ElementClickMixin(PolymerElement) {

ready() {
super.ready();
this.registerMouse(this._config);
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of the element click mixin now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still handles actually picking and executing the actions, which could be done in the elements themselves, but that's some code duplication...

It's actually restored to almost what it was before the first long-press PR.

Copy link
Member

Choose a reason for hiding this comment

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

nevermind saw you updated it already.

@balloob balloob merged commit 8cbd667 into home-assistant:master Oct 26, 2018
@ghost ghost removed the in progress label Oct 26, 2018
fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Oct 26, 2018
fabaff pushed a commit to home-assistant/home-assistant.io that referenced this pull request Nov 1, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
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