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

fix: Fixed FileWidget to work across all themes #3472

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

heath-freenome
Copy link
Member

Reasons for making this change

Fixes #2095 by making FileWidget use the BaseInputTemplate for rendering the file input

  • Updated @rjsf/utils to fix two small bugs found while fixing the issue:
    • dataURItoBlob() was updated to deal with parsing errors in atob() with malformed URL and to decode the name to a human-readable string - Added additional tests to verify the fixes
    • replaceStringParameters() was updated to split using a regex to deal with situations where the params themselves contain replaceable character identifiers
      • Added an additional test to verify the bug was fixed
  • Updated @rjsf/core to switch FileWidget so that it uses the BaseInputTemplate to render the input, passing a new onChangeOverride function to deal with file events
    • Updated BaseInputTemplate to favor onChangeOverride rather than the default onChange handler it builds
  • Updated the BaseInputTemplate in all of the themes to also favor onChangeOverride if specified over the default onChange handler
    • Updated the snapshots in all themes to reflect the improved FileWidget use of BaseInputTemplate in all themes
  • Updated @rjsf/bootstrap-4 to delete the FileWidget in favor of the improved core one
  • Updated @rjsf/material-ui and @rjsf/mui to have the BaseInputTemplate support the automatic shrink of the label when the type of the input is date, datetime-local and file
    • Also removed the DateWidget and DateTimeWidget in both themes since they were only providing the shrink behavior which is now encapsulated in BaseInputTemplate
  • Updated the CHANGELOG.md file accordingly

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Copy link
Contributor

@nickgros nickgros left a comment

Choose a reason for hiding this comment

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

Like the fix but not sure about the approach, have some suggestions but not sure if they are what we want either

return { blob, name };
return { blob, name };
} catch (error) {
return { blob: { size: 0, type: (error as Error).message }, name: dataURI };
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming .blob.type is shown to the user in the case of an error?

{...props}
disabled={disabled || readonly}
type="file"
onChangeOverride={handleChange}
Copy link
Contributor

Choose a reason for hiding this comment

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

How were you able to supply this onChangeOverride prop without adding it to the WidgetProps type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is file upload the only case that would benefit from this style?

Having both onChange and onChangeOverride seems like a confusing API, but I'm not sure if it should be clarified with better prop names or if it should have a different design.

Copy link
Member Author

Choose a reason for hiding this comment

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

How were you able to supply this onChangeOverride prop without adding it to the WidgetProps type?

WidgetProps extends GenericObjectType

const handleTextChange = onChangeOverride
? onChangeOverride
: ({ target }: React.ChangeEvent<HTMLInputElement>) =>
onChange(target.value === "" ? options.emptyValue : target.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This default onChange behavior is duplicated across all themes. Might make more sense to refactor to utils, e.g.

// Is there a better name?
export default function useOnEventTargetStringChange<
  T = any,
  S extends StrictRJSFSchema = RJSFSchema,
  F extends FormContextType = any
>(
  options: WidgetProps<T, S, F>["options"],
  onChange: WidgetProps<T, S, F>["onChange"]
) {
  return useCallback(
    ({ target: { value } }: React.ChangeEvent<HTMLInputElement>) =>
      onChange(value === "" ? options.emptyValue : value),
    [onChange, options.emptyValue]
  );
}

I am leaning towards the idea that this logic should be extracted from BaseInputTemplate altogether, since it doesn't work for all cases. Then we wouldn't need an onChangeOverrides prop. The downside is that we would have to pass this in almost every time we use BaseInputTemplate:

export default function CustomWidget() {
  const { onChange: _onChange, value, options, registry } = props;
  const BaseInputTemplate = getTemplate<"BaseInputTemplate", T, S, F>(
    "BaseInputTemplate",
    registry,
    options
  );
  const onChange = useOnEventTargetStringChange(options, _onChange)
  return (
    <BaseInputTemplate
      {...props}
      onChange={onChange}
    />
  );
}

I'm not sure if this verbosity + inversion is worth it for one (known) edge case...WDYT?

Copy link
Contributor

@nickgros nickgros Mar 6, 2023

Choose a reason for hiding this comment

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

A lower-lift option is to extend the default logic to also handle files (possibly being aware of the input type).

export default function useOnChangeEvent<
  T = any,
  S extends StrictRJSFSchema = RJSFSchema,
  F extends FormContextType = any
>(
  options: WidgetProps<T, S, F>["options"],
  onChange: WidgetProps<T, S, F>["onChange"],
  isFileInput?: boolean
) {
  return useCallback(
    (event: React.ChangeEvent<HTMLInputElement>) => {
      if (isFileInput) {
	// ...
      } else {
        onChange(value === "" ? options.emptyValue : value)
      }
    },
    [onChange, options.emptyValue, isFileInput]
  );
}

We could use this across BaseInputTemplates, so it wouldn't be necessary to do the larger refactor to pull this declaration out of BaseInputTemplate that I suggested in the above comment.

function BaseInputTemplate() {
  //...
  const handleTextChange = useOnChangeEvent(options, onChange, type === 'file')
  //...
}

Comment on lines +73 to +76
const DisplayInputLabelProps = TYPES_THAT_SHRINK_LABEL.includes(type)
? {
...InputLabelProps,
shrink: true,
}
: InputLabelProps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice simplification

Fixes rjsf-team#2095 by making `FileWidget` use the `BaseInputTemplate` for rendering the file input
- Updated `@rjsf/utils` to fix two small bugs found while fixing the issue:
  - `dataURItoBlob()` was updated to deal with parsing errors in `atob()` with malformed URL and to decode the name to a human-readable string
    - Added additional tests to verify the fixes
  - `replaceStringParameters()` was updated to split using a regex to deal with situations where the params themselves contain replaceable character identifiers
    - Added an additional test to verify the bug was fixed
- Updated `@rjsf/core` to switch `FileWidget` so that it uses the `BaseInputTemplate` to render the input, passing a new `onChangeOverride` function to deal with file events
  - Updated `BaseInputTemplate` to favor `onChangeOverride` rather than the default `onChange` handler it builds
- Updated the `BaseInputTemplate` in all of the themes to also favor `onChangeOverride` if specified over the default `onChange` handler
  - Updated the snapshots in all themes to reflect the improved `FileWidget` use of `BaseInputTemplate` in all themes
- Updated `@rjsf/bootstrap-4` to delete the `FileWidget` in favor of the improved `core` one
- Updated `@rjsf/material-ui` and `@rjsf/mui` to have the `BaseInputTemplate` support the automatic `shrink` of the label when the `type` of the input is `date`, `datetime-local` and `file`
  - Also removed the `DateWidget` and `DateTimeWidget` in both themes since they were only providing the `shrink` behavior which is now encapsulated in `BaseInputTemplate`
- Updated the `CHANGELOG.md` file accordingly
…playLabel` feature like all the rest of the themes do
@nickgros nickgros merged commit 610f0c8 into rjsf-team:main Mar 6, 2023
@heath-freenome heath-freenome deleted the fix-2095 branch March 6, 2023 20:57
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.

[bootstrap-4] File uploads result in error ".file should match format "data-url""
2 participants