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

New component: ha-expansion-panel #6789

Merged
merged 25 commits into from
Oct 14, 2020

Conversation

Misiu
Copy link
Contributor

@Misiu Misiu commented Sep 4, 2020

Breaking change

Proposed change

bc6ec0d6b5943b004415fb8d7cf321c0

This adds a new component ha-expansion-panel. I want to use it with the time condition editor.

Supported parameters:

  • expanded
  • outlined

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@bramkragten
Copy link
Member

You should probably use a <ha-card> as basis.

@Misiu
Copy link
Contributor Author

Misiu commented Sep 4, 2020

@bramkragten so I should extend ha-card?
I can try.

@bramkragten
Copy link
Member

@bramkragten so I should extend ha-card?
I can try.

No, not extend, just use it in your render.

@zsarnett
Copy link
Contributor

zsarnett commented Sep 4, 2020

Should we use HA-card?

Or should this be the guts of the functionality and can be outside of a <ha-card> or can be placed with in one

@bramkragten

}

.summary-content {
margin: 12px 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Too much margin. Specifically on the bottom. The content is not close enough to the summary title

Comment on lines 90 to 95
line-height: 1.5;
letter-spacing: 0.00938em;
font-size: 0.9375rem;
font-family: "Roboto", "Helvetica", "Arial", sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
line-height: 1.5;
letter-spacing: 0.00938em;
font-size: 0.9375rem;
font-family: "Roboto", "Helvetica", "Arial", sans-serif;
line-height: 1.2;
text-overflow: ellipsis;
overflow: hidden;

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 think ellipsis needs display, white-space an width properties, ref: https://stackoverflow.com/a/7993098/965722
I have a problem with width because it needs to be calculated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled your code and editted it. This is needed and I was able to add it.

Or you can just make it a lot and have the element using it do it :)

margin: 12px 0;
display: flex;
flex-grow: 1;
transition: margin 150ms cubic-bezier(0.4, 0, 0.2, 1) 0ms;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
transition: margin 150ms cubic-bezier(0.4, 0, 0.2, 1) 0ms;
transition: margin 150ms cubic-bezier(0.4, 0, 0.2, 1) 0ms;
overflow: hidden;

.summary {
display: flex;
padding: 0px 16px;
min-height: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

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

actually its this that is too large that is making it not close enough to the content

@zsarnett
Copy link
Contributor

zsarnett commented Sep 9, 2020

We should use javascript to set the max-height on the element to the scroll-height and then transition the max height instead of the height, either using CSS or Javascript and requestAnimationFrame

@Misiu Misiu force-pushed the ha-expansion-panel branch from c459364 to 2c9f2da Compare September 9, 2020 07:29
@zsarnett
Copy link
Contributor

zsarnett commented Sep 9, 2020

I really like this Component. I already have a branch that is using it 😄

@Misiu
Copy link
Contributor Author

Misiu commented Sep 9, 2020

@zsarnett that's why I want this to be part of HA :) just need to polish this a bit more.

@zsarnett
Copy link
Contributor

Any update on this? Would love to be able to get this in so it can start to be used

@Misiu
Copy link
Contributor Author

Misiu commented Sep 20, 2020

@zsarnett sadly no updates for now. I'll try to move things forward next week.
I'll optimize CSS and address your comments.
Not sure how to use ha-card in the code, but that will be next step.

@zsarnett
Copy link
Contributor

zsarnett commented Sep 20, 2020

I wouldn't use ha card.

But all good. Just making sure it hasn't been forgotten

@Misiu Misiu force-pushed the ha-expansion-panel branch from 5ea366d to 228ff6b Compare September 30, 2020 11:27

private _handleTransitionEnd() {
if (this.expanded) {
this._container.style.height = "auto";
Copy link
Member

@bramkragten bramkragten Oct 2, 2020

Choose a reason for hiding this comment

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

This works right?

Suggested change
this._container.style.height = "auto";
this._container.style.removeProperty("height");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this works perfectly. I've simplified the logic a bit thanks to this.
The last thing I'd like to remove is setTimeout, but collapse doesn't work without this.

@bramkragten
Copy link
Member

We can merge this as soon as we have implemented it somewhere.

@Misiu
Copy link
Contributor Author

Misiu commented Oct 2, 2020

We can merge this as soon as we have implemented it somewhere.

I want to implement this in the time condition editor.
My idea is to split time and date into two panels and expand them only if something is filled.
So I'll show the time part if after or before is filled, same for the weekday section. I want to show it only if the user has changed something (selected a day).
I can create a PR to show what I have in mind.

Also, @zsarnett had usage for this component.

@donkawechico
Copy link
Contributor

This is awesome! I also have use for it -- I want to make sequence steps collapsible and show their alias property as a short-hand description.

@balloob
Copy link
Member

balloob commented Oct 14, 2020

Why are we collapsing parts of a condition instead of the whole condition?

And when we collapse a condition, we should make sure that a summary of that condition is put in the name.

Ie "Time trigger at 10:01"

Once we have such summaries, we can just automatically collapse everything, making it easier to see what's going on in an automation.

@Misiu
Copy link
Contributor Author

Misiu commented Oct 14, 2020

@balloob this PR only adds a new component. No usage. I'm planning to add usage in a separate PR.
The GIF in the first message is just to show how the component looks like.
My motivation was to split time condition into two parts, but I agree that this can be applied to all conditions.

@bramkragten bramkragten mentioned this pull request Oct 21, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 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.

Add expansion panel component
7 participants