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.

Sign up for GitHub

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

Jump to bottom

fix(input-date-picker): no longer close while navigating months with chevron actions #11525

Merged
merged 5 commits into from
Feb 19, 2025
Merged

fix(input-date-picker): no longer close while navigating months with chevron actions #11525

merged 5 commits into from
Feb 19, 2025

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Feb 12, 2025 •
edited
Loading

Related Issue: #11534

Summary

No longer close input-date-picker while navigating between months with chevron actions & min/max are assigned adjacent month values.

@anveshmekala anveshmekala added the regression Issues that are caused by changes in a release, but were working before that. label Feb 12, 2025
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Feb 12, 2025
@geospatialem geospatialem added this to the 3.0.0 Patch - 2025-02-13 milestone Feb 12, 2025
@anveshmekala anveshmekala marked this pull request as ready for review February 13, 2025 22:49
@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 13, 2025
jcfranco
jcfranco approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

🚀

...ages/calcite-components/src/components/date-picker-month-header/date-picker-month-header.tsx Outdated
@@ -351,7 +351,13 @@ export class DatePickerMonthHeader extends LitElement {

if (isTargetLastValidMonth) {
if (!this.position) {
await (isDirectionLeft ? this.nextMonthAction.setFocus() : this.prevMonthAction.setFocus());
if (isDirectionLeft) {
this.nextMonthAction.disabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can you DRY this up?

const target = isDirectionLeft ? this.nextMonthAction : this.prevMonthAction;
target.disabled = false;
await target.setFocus();

driskull
driskull reviewed Feb 13, 2025
View reviewed changes
...ages/calcite-components/src/components/date-picker-month-header/date-picker-month-header.tsx Outdated
@@ -351,7 +351,13 @@ export class DatePickerMonthHeader extends LitElement {

if (isTargetLastValidMonth) {
if (!this.position) {
await (isDirectionLeft ? this.nextMonthAction.setFocus() : this.prevMonthAction.setFocus());
if (isDirectionLeft) {
this.nextMonthAction.disabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like disabled here is also being set in renderChevron(). Would this conflict with that? Maybe a state property can be used here to handle disabling the action?

Should the test cover this action being disabled?

Copy link
Contributor Author

@anveshmekala anveshmekala Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This would conflict with the disabled property in renderChevron( ) method. We may need two state properties to handle disable state for both actions. Could be a redo me thinks considering possible refactor.

Ideally this should be handled with component lifecyle. Tried awaiting on updateComplete but it invokes an update to the chevron action in time before the blur is invoked & bubbled up to input-date-picker.

Open to other suggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the test cover this action being disabled?

Don't think its required to assert on the attribute (possibly changing the approach in refactor) and the test covers visibility of date-picker.

Copy link
Contributor Author

@anveshmekala anveshmekala Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Added issue #11591 to investigate further.

Copy link
Member

Choose a reason for hiding this comment

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

I'll defer to @jcfranco for approval on this one. The code seems fine but could lead to conflicts since the disabled property is set here as well as with the VDOM on this element. It seems like only setting it via JSX is the safer route and should suffice.

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'll defer to @jcfranco for approval on this one. The code seems fine but could lead to conflicts since the disabled property is set here as well as with the VDOM on this element. It seems like only setting it via JSX is the safer route and should suffice.

Apologies i missed this before merging, had to refresh the tab for this comment to show up 😢

Copy link
Member

Choose a reason for hiding this comment

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

@anveshmekala Can you add a note to #11591 to make sure the above concern is addressed?

@anveshmekala anveshmekala marked this pull request as draft February 14, 2025 17:07
@anveshmekala anveshmekala marked this pull request as ready for review February 19, 2025 19:03
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 19, 2025
@anveshmekala anveshmekala mentioned this pull request Feb 19, 2025
Open
5 tasks
@anveshmekala anveshmekala merged commit 5472452 into dev Feb 19, 2025
14 checks passed
@anveshmekala anveshmekala deleted the anveshmekala/fix-input-date-picker-close-when-chevron-click branch February 19, 2025 20:17
benelan pushed a commit that referenced this pull request Feb 19, 2025
@anveshmekala @benelan
fix(input-date-picker): no longer close while navigating months with …
83d6a81 
…chevron actions (#11525)

**Related Issue:** #11534 

## Summary

No longer close `input-date-picker` while navigating between months with
chevron actions & min/max are assigned adjacent month values.
benelan pushed a commit that referenced this pull request Feb 19, 2025
@anveshmekala @benelan
fix(input-date-picker): no longer close while navigating months with …
acf5824 
…chevron actions (#11525)

**Related Issue:** #11534 

## Summary

No longer close `input-date-picker` while navigating between months with
chevron actions & min/max are assigned adjacent month values.
@calcite-admin calcite-admin mentioned this pull request Feb 19, 2025
Merged
benelan pushed a commit that referenced this pull request Feb 19, 2025
@calcite-admin
chore: release main (#11596)
0b0b89f 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 3.0.3</summary>

##
[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@3.0.2...@esri/calcite-components@3.0.3)
(2025-02-19)


### Bug Fixes

* **dialog, modal, popover, sheet:** Restore `focusTrapDisabled`
reactiveness
([#11586](#11586))
([a32dd7e](a32dd7e))
* **input-date-picker:** No longer close while navigating months with
chevron actions
([#11525](#11525))
([acf5824](acf5824))
* **slider:** Ensure histograms with non-zero min are displayed
correctly
([#11587](#11587))
([9bb32f6](9bb32f6))
* **tooltip:** Close tooltips on prevented pointer move events
([#11557](#11557))
([470df20](470df20))
</details>

<details><summary>@esri/calcite-components-react: 3.0.3</summary>

##
[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@3.0.2...@esri/calcite-components-react@3.0.3)
(2025-02-19)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize components versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from 3.0.2 to 3.0.3
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
benelan pushed a commit that referenced this pull request Feb 19, 2025
@calcite-admin @benelan
chore: release main (#11596)
559e684 
🤖 I have created a release *beep* *boop*
---

<details><summary>@esri/calcite-components: 3.0.3</summary>

[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@3.0.2...@esri/calcite-components@3.0.3)
(2025-02-19)

* **dialog, modal, popover, sheet:** Restore `focusTrapDisabled`
reactiveness
([#11586](#11586))
([a32dd7e](a32dd7e))
* **input-date-picker:** No longer close while navigating months with
chevron actions
([#11525](#11525))
([acf5824](acf5824))
* **slider:** Ensure histograms with non-zero min are displayed
correctly
([#11587](#11587))
([9bb32f6](9bb32f6))
* **tooltip:** Close tooltips on prevented pointer move events
([#11557](#11557))
([470df20](470df20))
</details>

<details><summary>@esri/calcite-components-react: 3.0.3</summary>

[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@3.0.2...@esri/calcite-components-react@3.0.3)
(2025-02-19)

* **@esri/calcite-components-react:** Synchronize components versions

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from 3.0.2 to 3.0.3
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@benelan benelan mentioned this pull request Feb 19, 2025
Merged
benelan added a commit that referenced this pull request Feb 19, 2025
@benelan @calcite-admin
chore: release main (#11596)
5ca8878 
🤖 I have created a release *beep* *boop*
---

<details><summary>@esri/calcite-components: 3.0.3</summary>


[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@3.0.2...@esri/calcite-components@3.0.3)
(2025-02-19)

* **dialog, modal, popover, sheet:** Restore `focusTrapDisabled`
reactiveness
([#11586](#11586))

([a32dd7e](a32dd7e))
* **input-date-picker:** No longer close while navigating months with
chevron actions
([#11525](#11525))

([acf5824](acf5824))
* **slider:** Ensure histograms with non-zero min are displayed
correctly
([#11587](#11587))

([9bb32f6](9bb32f6))
* **tooltip:** Close tooltips on prevented pointer move events
([#11557](#11557))

([470df20](470df20))
</details>

<details><summary>@esri/calcite-components-react: 3.0.3</summary>


[3.0.3](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@3.0.2...@esri/calcite-components-react@3.0.3)
(2025-02-19)

* **@esri/calcite-components-react:** Synchronize components versions

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from 3.0.2 to 3.0.3
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See

[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Calcite Admin <calcite-admin@esri.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

@driskull driskull Awaiting requested review from driskull

Assignees

@anveshmekala anveshmekala

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing. regression Issues that are caused by changes in a release, but were working before that.
Projects
None yet
Milestone
3.0.0 Patch - 2025-02-19
Development

Successfully merging this pull request may close these issues.
4 participants
@anveshmekala @jcfranco @driskull @geospatialem