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

fix(client): electron app doesn't respect reflected properties #2399

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

daniellacosse
Copy link
Contributor

@daniellacosse daniellacosse commented Feb 18, 2025

Turns out the electron app wasn't respecting lit's reflected properties for some reason (Polymer?) - they would show in the storybook but not in the app. I discovered this while forcing in the necessary data for the marketing screenshots. Here it is properly working:

Screenshot 2025-02-18 at 6 08 46 PM Screenshot 2025-02-18 at 6 11 16 PM

@daniellacosse daniellacosse requested a review from a team as a code owner February 18, 2025 23:11
Copy link
Contributor

@jyyi1 jyyi1 left a comment

Choose a reason for hiding this comment

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

This is odd, cuz both electron and lit have been used by many people. Maybe updating some of the other dependencies would help? (we can do it in another PR if updating Electron is the solution)

@daniellacosse
Copy link
Contributor Author

daniellacosse commented Feb 19, 2025

This is odd, cuz both electron and lit have been used by many people. Maybe updating some of the other dependencies would help? (we can do it in another PR if updating Electron is the solution)

Yeah, hence why I didn't prioritize testing this specifically outside the storybook.

My initial guess was that maybe Polymer is interfering with this somehow, but one of these instances occur on a Lit component inside of other Lit component. Surely Polymer doesn't interfere beyond the shadow boundary, if at all?

Anyway, I think we should fix this first and then get to the bottom of it. Gotta release.

Copy link
Contributor

@sbruens sbruens left a comment

Choose a reason for hiding this comment

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

Can you create an issue for us to look into this later and reference it in the TODO comments?

@daniellacosse daniellacosse enabled auto-merge (squash) February 19, 2025 14:06
@daniellacosse daniellacosse merged commit 51c811b into master Feb 19, 2025
25 checks passed
@daniellacosse daniellacosse deleted the daniellacosse/polyfill-reflected-properties branch February 19, 2025 14:24
daniellacosse added a commit that referenced this pull request Feb 20, 2025
* fix(client): electron app doesn't respect reflected properties

* Update bandwidth.ts

* Update index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants