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

Implement form.io email component type #17

Merged
merged 6 commits into from
Aug 28, 2023

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented May 11, 2023

Closes #18

design decisions to make/think about

-> pushed preview validation to separate issue #19

tasks

  • DRY/refactor code duplication with textfield
  • add interaction tests

@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch from 3186194 to 52e954f Compare May 11, 2023 21:08
@sergei-maertens
Copy link
Member Author

Reference of formio:

image

  • individual item validation errors are displayed as "entire component" errors
  • better UX is errors per field where they occur imo

sergei-maertens added a commit that referenced this pull request May 13, 2023
Centralized common schema validation such as label + key, in
preparation for complexer component.type specific validations, using
zod unions and intersections.
sergei-maertens added a commit that referenced this pull request May 13, 2023
This allows for re-using the error list between wrapper and 'atomic'
components, especially for the multiple: true variant.

NOTE: form.io itself always displays the errors at the wrapper level,
but it should be better UX/a11y to display the errors precisely with
the invalid input. There is definitely more to gain in terms of a11y
here, e.g. with aria-describedby etc.
sergei-maertens added a commit that referenced this pull request May 13, 2023
* Added stories for configuration + preview
* Implemented defaultValue builder form schema validation, taking into
  account the 'multiple' flag.
* Added email to the supported components registry

TODO (later)

* validation in preview component
* update preview when value of defaultValue in builder form changes
@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch from a094d15 to 4d06d35 Compare May 13, 2023 14:40
sergei-maertens added a commit that referenced this pull request May 13, 2023
Centralized common schema validation such as label + key, in
preparation for complexer component.type specific validations, using
zod unions and intersections.
sergei-maertens added a commit that referenced this pull request May 13, 2023
This allows for re-using the error list between wrapper and 'atomic'
components, especially for the multiple: true variant.

NOTE: form.io itself always displays the errors at the wrapper level,
but it should be better UX/a11y to display the errors precisely with
the invalid input. There is definitely more to gain in terms of a11y
here, e.g. with aria-describedby etc.
sergei-maertens added a commit that referenced this pull request May 13, 2023
* Added stories for configuration + preview
* Implemented defaultValue builder form schema validation, taking into
  account the 'multiple' flag.
* Added email to the supported components registry

TODO (later)

* validation in preview component
* update preview when value of defaultValue in builder form changes
@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch 2 times, most recently from 779732e to 90ed545 Compare May 13, 2023 14:41
@sergei-maertens sergei-maertens marked this pull request as ready for review May 13, 2023 14:42
Copy link
Contributor

@SilviaAmAm SilviaAmAm left a comment

Choose a reason for hiding this comment

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

I noticed while playing with storybook that if you do this:

  • Let the story do its play tests
  • Uncheck the 'multiple' checkbox
    Then the defaultValue remains:
"defaultValue": [
    "invalid",
    ""
  ]

And it displays as:
Screenshot from 2023-05-31 17-52-45
(not sure if this is something that will be fixed in other PRs)

Comment on lines +157 to +158
show: undefined,
when: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are both null to start with in Formio? Not sure if it is a problem

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, but their own type definitions don't allow for null and using null violates the typescript types, which breaks compilation. A typical example of form.io not sticking to their own semantics 😬

return (
<TextField
name={name}
multiple={!!multiple}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't multiple already a bool? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

it can also be undefined as it's an optional prop for the preview

@sergei-maertens
Copy link
Member Author

I noticed while playing with storybook that if you do this:

  • Let the story do its play tests
  • Uncheck the 'multiple' checkbox
    Then the defaultValue remains:
"defaultValue": [
    "invalid",
    ""
  ]

And it displays as: Screenshot from 2023-05-31 17-52-45 (not sure if this is something that will be fixed in other PRs)

Yep, it's a known issue and tracked in #21

@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch from 6c35f8c to bee7fb9 Compare June 7, 2023 15:35
@sergei-maertens sergei-maertens marked this pull request as draft June 7, 2023 15:40
@sergei-maertens
Copy link
Member Author

Back to draft - I'll get the refactors in a separate PR and sort out adding the typescript types first.

* Added stories for configuration + preview
* Implemented defaultValue builder form schema validation, taking into
  account the 'multiple' flag.
* Added email to the supported components registry

TODO (later)

* validation in preview component
* update preview when value of defaultValue in builder form changes
@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch from a02ba2e to b5a7b1b Compare August 25, 2023 15:46
<Tabs>
<TabList>
<Tab
hasErrors={hasAnyError(
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't find this particularly elegant, but I don't know how to fix it either. The field name (and thus the error key) is deeply nested, and the field itself does not know in which tab it's being displayed.

@sergei-maertens sergei-maertens marked this pull request as ready for review August 25, 2023 15:55
@sergei-maertens sergei-maertens force-pushed the feature/component-type-email branch from 47de2ec to 941ea59 Compare August 28, 2023 13:16
@sergei-maertens sergei-maertens merged commit a964b97 into main Aug 28, 2023
@sergei-maertens sergei-maertens deleted the feature/component-type-email branch November 22, 2023 15:13
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.

Implement builder form for email component type
2 participants