Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
290 stars 75 forks source link

Calcite-Input-Time-Picker: UI doesn't refresh when updating step property #6039

Closed JonUihlein closed 1 year ago

JonUihlein commented 1 year ago

Actual Behavior

CalciteInputTimePicker UI doesn't update after changing the step property between different values. Seconds remain visible (or invisible) in the input depending on the previous step value until a subsequent interaction. Additionally, the time popover never updates to show the correct configuration (separate bug `CalciteTimePicker).

This bug may also affect other configurations as well (e.g. hours/minutes only) but I didn't test those configurations.

Using specific values, changing step to 1 (from default 60) should cause the CalciteInputTimePicker to show "seconds" in the input. This does not happen until a subsequent interaction (clicking the input). Additionally, the time popover never updates to show seconds, even on subsequent interactions (open/close).

Two different bugs to address: The <calcite-input> used inside CalciteInputTimePicker does not update after the step value changes, until an interaction. I think this property may need a watcher to trigger a UI update.

The CalciteTimePicker contained in the <calcite-popover> inside CalciteInputTimePicker does not update after the step value on the parent component changes. This is also reproducible using a stand-alone CalciteTimePicker, but it may need additional logic to react to step changes on the parent component.

Expected Behavior

Seconds/minutes should be visible/invisible depending on step.

Time Picker Popover should correctly reflect current step value.

Reproduction Sample

CalciteInputTimePicker (button): https://jsbin.com/fisopopavi/edit?html,output

CalciteTimePicker (button): https://jsbin.com/piqecanefi/edit?html,output

(old) CalciteInputTimePicker (setTimeout): https://jsbin.com/pupiviseza/edit?html,output

Reproduction Steps

1) Create CalciteInputTimePicker 2) Update CalciteInputTimePicker.step from 60 (default) to 1 3) Notice the UI does not update 4) Click the input to trigger the popover 5) Notice the popover does not show seconds 6) Click away 7) Input should now display time with seconds 8) Click the input to trigger the popover 9) Notice the popover still does not show seconds

Reproduction Version

1.0.0-next.665

Relevant Info

No response

Regression?

No response

Impact

Noticed this while integrating CalciteInputDatePicker and CalciteInputTimePicker into our FeatureForm widget.

Esri team

ArcGIS API for JavaScript

geospatialem commented 1 year ago

@JonUihlein The timing might be related to the whenDefined() method of the customElementRegistry where once the component is defined, JavaScript can be executed thereafter.

A Codepen example, and a JavaScript snippet below:

customElements.whenDefined("calcite-input-time-picker").then(() => document.querySelector("calcite-input-time-picker").setAttribute("step", 1));
JonUihlein commented 1 year ago

@geospatialem - I can confirm that the element is already defined and the UI still does not refresh after changing the value for step.

The above testapp uses setTimeout for convenience but even using a generic button to update step after the app is loaded and the component is defined does not trigger a UI refresh.

Test app using a generic button: https://jsbin.com/fisopopavi/edit?html,output

Additionally, looking at the source for CalciteInputDatePicker, I don't see anything that would trigger a UI update after the value changes, but there could be some magic under the hood that I'm unaware of.

JonUihlein commented 1 year ago

@geospatialem - follow up question that isn't specific to this issue:

Is it a recommended pattern to use customElements.whenDefined() to ensure the components are always ready? It looks like we haven't had to use this for calcite components on the JSAPI side before.

We actually do use customElements.whenDefined() in the FeatureTable widget but it's because we're injecting custom styles into a third party component's shadowDom (which is hacky and we would never want to do for any calcite components).

*Edit - link removed.

geospatialem commented 1 year ago

I can confirm that the element is already defined and the UI still does not refresh after changing the value for step.

@JonUihlein Ah, yes, thanks for the clarity - was focused on the initial load, see the issue when trying to update.

For devs: This might have a similar fix needed to accommodate the input-date-picker internal value update in #5886, where the lang property isn't getting updated in a similar way (but unrelated as it applies to a regression in next.621.

geospatialem commented 1 year ago

Is it a recommended pattern to use customElements.whenDefined() to ensure the components are always ready? It looks like we haven't had to use this for calcite components on the JSAPI side before.

Great question, it wouldn't be a pattern for all instances - but could be useful depending on the use case.

For instance, when a component is dependent on another, such as displaying a loader until a component loads on the page:

(async () => {
  await customElements.whenDefined("calcite-alert");
  await document.querySelector("calcite-alert").componentOnReady();
  const loaderEl = document.querySelector("calcite-loader");
  loaderEl.setAttribute("hidden", true);
})();
JonUihlein commented 1 year ago

@geospatialem - Spent some time digging around the source.

It seems like the issue is calciteInputTimePicker.calciteTimePickerEl.showSecond is never recomputed when step changes. I think adding a watcher for this prop to calcite-time-picker should fix that problem; changing this value manually at the app level causes the UI to render as expected.

In the meantime, I'm attempting to use your suggestion of: customElements.whenDefined("calcite-input-time-picker") but it looks like the internal calcite-time-picker isn't ready when this fires, so assigning properties to the parent isn't reflected on the child component. I also tried using customElements.whenDefined("calcite-time-picker") to see if that would trigger when using the parent "input" component but it never fires.

I also tried using component.componentOnReady() but got a runtime error: input.componentOnReady is not a function

jcfranco commented 1 year ago

I also tried using component.componentOnReady() but got a runtime error: input.componentOnReady is not a function

Ah, this is because componentOnReady is only available for the lazy-loaded output target. Could you try the following?

(async () => {
  await customElements.whenDefined("calcite-alert");
  await (new Promise(resolve => requestAnimationFrame(resolve))); // allow component to render
  const loaderEl = document.querySelector("calcite-loader");
  loaderEl.setAttribute("hidden", true);
})();

This is sort of based on Ionic's util to help deal with both.

jcfranco commented 1 year ago

@eriklharper Stealing this one. 🚓💨💨💨

github-actions[bot] commented 1 year ago

Installed and assigned for verification.

geospatialem commented 1 year ago

Verified on master.

verify-ui-refresh