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

Replace polymer paper-slider #18168

Merged
merged 9 commits into from
Oct 16, 2023
Merged

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Oct 9, 2023

Proposed change

Replace polymer paper-slider and use the brand new material web components.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

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:

@github-actions github-actions bot added Dependencies Pull requests that update a dependency file Demo Related to frontend demo content Design Related to Home Assistant design gallery labels Oct 9, 2023
@bramkragten
Copy link
Member

In the more info dialogs, the slider is very small and the tooltip is cutoff:
CleanShot 2023-10-10 at 15 18 27
Expected:
CleanShot 2023-10-10 at 15 19 37

Also the service is never called when the value is changed with the slider.

@silamon
Copy link
Contributor Author

silamon commented Oct 10, 2023

I got it fixed but the cutoff is due to the dialog. In other places there's no cutoff.
There are still more places where the value isn't picked up. It needs some more testing.

@bramkragten bramkragten marked this pull request as draft October 11, 2023 10:21
@bramkragten
Copy link
Member

There are still more places where the value isn't picked up. It needs some more testing.

Marked as draft for now

@silamon silamon marked this pull request as ready for review October 11, 2023 16:18
@silamon
Copy link
Contributor Author

silamon commented Oct 11, 2023

There are still more places where the value isn't picked up. It needs some more testing.

Marked as draft for now

I've fixed all places yesterday. Now tested more, and the only thing I can still notice is the cutoff pin in the more info dialog.

.min=${this.min}
.max=${this.max}
.step=${this.step}
labeled=${this.labeled}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
labeled=${this.labeled}
.labeled=${this.labeled}

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 this one is incorrect. Labeled should be passed to ha-slider, it should be attached to the tag.

<ha-slider {}></ha-slider> => no pin on the slider
<ha-slider labeled {}></ha-slider => pin on the slider

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Demo Related to frontend demo content Dependencies Pull requests that update a dependency file Design Related to Home Assistant design gallery hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants