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

Engine support for <trigger> #216

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Engine support for <trigger> #216

merged 4 commits into from
Sep 19, 2024

Conversation

eyelidlessness
Copy link
Member

@eyelidlessness eyelidlessness commented Sep 13, 2024

Closes # (nothing is coming up in search, do we have an issue I missed?)

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS)
  • Safari (iOS)
  • Not applicable

The bit of UI implemented is just the minimum necessary to make types pass, and shows the engine side working. I doubt there's much point in cross-browser testing it.

What else has been done to verify that this works as intended?

A few minimal scenario tests. If there's any more nuance specific to the control, I'd be happy to expand testing further.

Why is this the best possible solution? Were any other approaches considered?

Pretty much just expands on current approach to similar things. Any kind of different approach would hopefully be more holistic.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Since the change is additive, there's not much room for regression there.

Do we need any specific form for testing your changes? If so, please attach one.

Any form with a <trigger>

What's changed

As described in the title, this adds engine support for <trigger>.

XForms represents a trigger value as 'OK' | ''. As discussed in Slack, I originally considered representing the runtime value state for this as true | null, but that was pretty awkward to actually use. Eventually landed on just a plain boolean value, which is pretty much semantically equivalent as well.

Some of the scenario APIs for setting (Scenario.answer) and comparing/asserting (ComparableAnswer) value state have been expanded to specifically handle this boolean case. There will likely be more expansion as we support other data types. In this case I wanted to keep the change relatively narrow, not overly speculating on what those other data types' implementation will look like, but also trying not to make it so trigger-specific as to make those anticipated changes more awkward when we address them.

Lastly, I made a very small change on the UI side. As mentioned above, the intent is to make the minimal change necessary for type checks to pass and to demonstrate the functionality working from the engine side. Otherwise, TriggerControl.vue is basically just copypasta from similar aspects of existing components, with minor tweaks to address the specific semantics of the control (i.e. effectively equivalent to XForms <input> with a boolean value type, so ~equivalent to HTML <input type="checkbox">) and likely form structures utilizing it (i.e. the <label> is more likely be associated with the field rather than the value, quite unlike HTML <input type="checkbox"> + <label>).

Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: 6fb1054

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@getodk/scenario Minor
@getodk/web-forms Minor
@getodk/xforms-engine Minor
@getodk/ui-solid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The intent here isn’t to suggest any particular design, just to:

1. Satisfy the type checker
2. Show enough integration that it’s clear the engine side is working as intended
@sadiqkhoja sadiqkhoja merged commit 17e5b25 into main Sep 19, 2024
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants