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

gpld-conditional-limits.php: Added a new snippet to enable conditional date limits. #884

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

barthc
Copy link
Contributor

@barthc barthc commented Sep 7, 2024

Context

⛑️ Ticket(s): https://secure.helpscout.net/conversation/2690682399/70534

Summary

Added a new snippet to enable conditional date limits.

Copy link

github-actions bot commented Sep 7, 2024

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against d4c93d7

@barthc barthc force-pushed the barth/add/70534-conditional-limit-dates branch 3 times, most recently from cdfac92 to 6a11d70 Compare September 10, 2024 07:31
Copy link
Contributor

@spivurno spivurno left a comment

Choose a reason for hiding this comment

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

Dude!! Very excited about this. Some feedback:

https://www.loom.com/share/279be099b3024bc6a9b1b04701300eb3

  1. Set settings by default so they don’t have to manually mapped if they’re unaffected.
    • Also, some properties are not supported (e.g. inlineDatepicker).
  2. Need to output script in the footer. See: https://github.com/gravitywiz/snippet-library/blob/master/gw-snippet-template.php#L83C2-L91C3
  3. Need to enqueue conditional_logic.js if this script is used.

Update: Oh, and I forgot to mention, we should probably clear out the value if it is not a valid date based on the new configuration when a new conditional configuration is applied.

@barthc barthc force-pushed the barth/add/70534-conditional-limit-dates branch 2 times, most recently from 2c31932 to d11cfa5 Compare September 16, 2024 14:08
@spivurno
Copy link
Contributor

@barthc Getting close!

A few lingering issues:
https://www.loom.com/share/504667b49e644a35876c3156188ea573

  • Let's clear the date when configuration changes.
  • "Invalid Date" warning does not show up when conditional configuration is applied.
  • Invalid dates are not triggering validation errors when conditional configuration is applied.

@barthc barthc force-pushed the barth/add/70534-conditional-limit-dates branch from d11cfa5 to d344efc Compare September 19, 2024 00:05
@barthc barthc force-pushed the barth/add/70534-conditional-limit-dates branch from d344efc to d4c93d7 Compare September 19, 2024 11:30
Copy link
Contributor

@spivurno spivurno left a comment

Choose a reason for hiding this comment

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

Working well!

One note: It's preferable to just make multiple commits during the review process rather than force pushing changes. It allows me to see what changed which makes code review a lot easier on subsequent reviews. 🙏

@barthc barthc merged commit d324431 into master Sep 20, 2024
3 checks passed
@barthc barthc deleted the barth/add/70534-conditional-limit-dates branch September 20, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants