AlaskaAirlines / auro-banner

Custom element that ....
https://auro.alaskaair.com/components/auro/banner
Apache License 2.0
0 stars 0 forks source link

auro-banner: unable to update ratio after first render #27

Closed geoffrich closed 2 years ago

geoffrich commented 3 years ago

Describe the bug

I am unable to update ratio after the component first renders.

To Reproduce

Steps to reproduce the behavior:

  1. Render the component
  2. Set ratio after first render, e.g. document.querySelector('auro-banner').ratio = "10:1".
  3. The component doesn't update and the proportions stay the same

Expected behavior

The component updates whenever ratio is changed.

Additional context

This is likely because the ratio is only calculated once, in firstUpdated.

This should be addressed before publishing the component to Auro (#10).

blackfalcon commented 3 years ago

What is this behavior expectation based on? What is a use case where a user would want to change the ratio on the fly?

geoffrich commented 3 years ago

As a general rule, I expect reactivity from components (in any framework) -- I should be able to update a prop and have the component respond. Unless there's a good reason not to, I think Auro elements should behave the same way. It makes our components more resilient.

blackfalcon commented 3 years ago

As a general rule, I expect reactivity from components (in any framework)

That is an assumption, but not a build spec or requirement. http://auro.alaskaair.com/generator/best-practice

What you are saying is that you have an expectation that all properties will require reflect: true or how would you assume that we decide what is reactive and what is not?

What does make sense to me is that any boolean value should be reactive for sure, but any property that accepts a string needs to have a reason for being reactive IMHO.

geoffrich commented 3 years ago

That is an assumption, but not a build spec or requirement. http://auro.alaskaair.com/generator/best-practice

Sure, it's an assumption, but I don't think it's a unique one. If there's a property I can set at runtime, I expect setting that property to cause the component to update. Heck, the Lit feature we are using is even called "reactive properties".

It's also useful for debugging -- I can grab the element in the console, set the property, and watch it update in real time.

What you are saying is that you have an expectation that all properties will require reflect: true or how would you assume that we decide what is reactive and what is not?

reflect: true has nothing to do with it. By reactive, I mean setting this problem will cause the component to react by updating its rendered output. Also updating the associated attribute is not part of reactivity.

What does make sense to me is that any boolean value should be reactive for sure, but any property that accepts a string needs to have a reason for being reactive IMHO.

This is a weird heuristic to me. What string values in Auro elements are not reactive? I can update auro-pane's date property, auro-flight's duration, or auro-input's helpText and the component updates right away. By making ratio not reactive, we create a weird discrepancy.

Similarly, for native HTML elements, pretty much every property I can think of updates the element when set -- there aren't properties that only work if you set them on first render.

blackfalcon commented 3 years ago

Offline conversation: The concept of supporting reactive properties really centers around the concept of not locking a derived value from user input into a static render method like firstUpdated()

The best practice should be: if you derive data from a property that the user can update, you should react to changes in that property.

~ @geoffrich

As a best practice, any value that is created within the scope of an element that is based on the input from a property, we should place that function within a method that will update post first render.

It is suggested that the updated() method is used for this. https://lit-element.polymer-project.org/guide/lifecycle#updated