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

Proper method to disable ColorPicker and TimestampTag rendering #493

Closed
nook24 opened this issue Oct 21, 2024 · 4 comments
Closed

Proper method to disable ColorPicker and TimestampTag rendering #493

nook24 opened this issue Oct 21, 2024 · 4 comments
Labels
question Further information is requested

Comments

@nook24
Copy link

nook24 commented Oct 21, 2024

In the past, there was a colorPicker: false and timestampTag: false option. I don't find anything like this anymore.

As workaround, I copied the default renderValue method and removed the ColorPicker and TimestampTag. If i understand the docs correct, that's the supposed way to achieve this?
https://github.com/josdejong/svelte-jsoneditor/tree/96e4179871a18c42edbac25d3d62548ed7fe2aa7?tab=readme-ov-file#onrendervalue

onRenderValue(props: RenderValueProps) {
      // https://github.com/josdejong/svelte-jsoneditor/blob/96e4179871a18c42edbac25d3d62548ed7fe2aa7/src/lib/plugins/value/renderValue.ts#L9-L78

      const renderers: RenderValueComponentDescription[] = [];
      const isEditing = props.isEditing;

      if (!isEditing && isBoolean(props.value)) {
          renderers.push({
              component: BooleanToggle,
              props: {
                  path: props.path,
                  value: props.value,
                  readOnly: props.readOnly,
                  onPatch: props.onPatch,
                  focus: props.focus
              }
          });
      }

      if (!isEditing) {
          renderers.push({
              component: ReadonlyValue,
              props: {
                  path: props.path,
                  value: props.value,
                  mode: props.mode,
                  readOnly: props.readOnly,
                  parser: props.parser,
                  normalization: props.normalization,
                  searchResultItems: props.searchResultItems,
                  onSelect: props.onSelect
              }
          });
      }

      return renderers;
  },

Is this really the correct way to disable the colorpicker and Timestamp renderer?

@josdejong
Copy link
Owner

josdejong commented Oct 23, 2024

Hi, yes that is indeed the right way, though in your example you should also handle the case when a value is being edited.

The onRenderValue gives much more flexibility. Downside is that customization can be verbose. We can improve on that though, for example by offering some presets, or having some utility function that allows you to more easily compose from the existing components, something like createOnRenderValue({ colorPicker: false, timestampTag: false }). Suggestions on how to best improve this are welcome.

It is quite verbose, though the gist of it boils down to the following. The only reason I specify all props individually is that the Svelte compiler gives warnings when passing an unknown prop to a component.

function onRenderValue(props: RenderValueProps): RenderValueComponentDescription[] {
  const renderers: RenderValueComponentDescription[] = []

  if (!props.isEditing && isBoolean(props.value)) {
    renderers.push({ component: BooleanToggle, props })
  }

  if (props.isEditing) {
    renderers.push({ component: EditableValue, props })
  }

  if (!props.isEditing) {
    renderers.push({ component: ReadonlyValue, props })
  }

  return renderers
}

@josdejong josdejong added the question Further information is requested label Oct 23, 2024
@josdejong
Copy link
Owner

josdejong commented Oct 23, 2024

I'm working on a migration to Svelte 5, and it turns out that Svelte 5 doesn't warning against unused properties anymore, which is great for this very use case. See this commit: e640046

This simplifies the built-in implementation of renderValue to:

export function renderValue(props: RenderValueProps): RenderValueComponentDescription[] {
  const renderers: RenderValueComponentDescription[] = []

  if (!props.isEditing && isBoolean(props.value)) {
    renderers.push({ component: BooleanToggle, props })
  }

  if (!props.isEditing && isColor(props.value)) {
    renderers.push({ component: ColorPicker, props })
  }

  if (props.isEditing) {
    renderers.push({ component: EditableValue, props })
  }

  if (!props.isEditing) {
    renderers.push({ component: ReadonlyValue, props })
  }

  if (!props.isEditing && isTimestamp(props.value)) {
    renderers.push({ component: TimestampTag, props })
  }

  return renderers
}

I'm really happy with that 🎉

@nook24
Copy link
Author

nook24 commented Oct 23, 2024

Hi @josdejong, thanks for coming back to me.

though in your example you should also handle the case when a value is being edited.

Thats correct, in my case I'm using the json editor in read only mode as I just want to display some JSON data in a more user friendly way.

I like your suggestion with the easy flags to disable a renderer.

createOnRenderValue({ colorPicker: false, timestampTag: false })

I think it is also better for future updates, as users can benefit from bug fixes/improvements in the original renderValue method without having to check for changes within new version of svelte-jsoneditor.

As the detection magic is enabled by default, I guess it would be nice to have some more intuitive way to disable it. Honestly it took me awhile to realize that I really have to replace the entire renderValue method.

How ever, maybe I have some rar edge case data which is why I had to disable the Timestamp and Color Picker in the first place.

The Svelte 5 implementation you have mentioned looks nice and clean :)
Please feel free to close the issue if you like. Others will eventually find this issue with the (current) method of how to disable render features.

Thanks for your time, I appreciate this.

@josdejong
Copy link
Owner

Thanks for your input! I'll leave the function for now, and we can look into a helper function if there is enough need for it.

I've added some more explanation in the docs: 8fded08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants