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

Support different classes for different controls #2239

Open
vanillajonathan opened this issue Jan 3, 2024 · 8 comments
Open

Support different classes for different controls #2239

vanillajonathan opened this issue Jan 3, 2024 · 8 comments

Comments

@vanillajonathan
Copy link

Is your feature request related to a problem? Please describe.

I cannot use the Bootstrap classes with the Vanilla renderer because the Vanilla renderer only lets you set one class for all input control types.

In Bootstrap the inputs should have the form-control class but for checkboxes it should be form-check-input instead.

Describe the solution you'd like

A way to configure different CSS classes for different input types.
https://github.com/eclipsesource/jsonforms/blob/master/packages/vue-vanilla/src/styles/defaultStyles.ts

Perhaps:

  control: {
    root: 'control',
    wrapper: 'wrapper',
    label: 'label',
    description: 'description',
    input: 'input',
    error: 'error',
    textarea: 'text-area',
    select: 'select',
    option: 'option',
  },

could be:

  control: {
    root: 'control',
    wrapper: 'wrapper',
    label: 'label',
    description: 'description',
    input: {
      color: 'form-control form-control-color',
      checkbox: 'form-input-check',
      radio: 'form-input-check',
      default: 'form-control'
    },
    error: 'error',
    textarea: 'text-area',
    select: 'select',
    option: 'option',
  },

Describe alternatives you've considered

Fork the Vanilla renderer.

Framework

Vue 3

RendererSet

Vanilla

Additional context

No response

@sdirix
Copy link
Member

sdirix commented Jan 4, 2024

Hi @vanillajonathan, Thanks for the suggestion. This definitely makes sense to me.

I'm wondering about the approach, whether we should split per property as suggested, i.e.

  control: {
    root: 'control',
    wrapper: 'wrapper',
    label: 'label',
    description: 'description',
    input: {
      color: 'form-control form-control-color',
      checkbox: 'form-input-check',
      radio: 'form-input-check',
      default: 'form-control'
    },
    error: 'error',
    textarea: 'text-area',
    select: 'select',
    option: 'option',
  },

or split at the root level, i.e.

  control: {
    checkbox: {
      root: 'control',
      wrapper: 'wrapper',
      label: 'label',
      description: 'description',
      error: 'error',
      input: 'input'
    },
    text: {
      root: 'control',
      wrapper: 'wrapper',
      label: 'label',
      description: 'description',
      error: 'error',
      input: 'input'
    },
  },

@davewwww
Copy link
Contributor

davewwww commented Feb 9, 2024

Hi,

is there anything i can to do here? i can't see a problem here?

You can already add classes via the uischemas options or customize everything directly via css selector.

{
"type": "Control",
"scope": "#/properties/text",
"options": {
  "styles": {
    "control": {
      "label": "label more classes",
      "textarea": "text-area more classes",
      "description": "description more classes"
  }
 }
}

@vanillajonathan
Copy link
Author

The uiSchema options doesn't seem like a good option because you should be able to render a form without specifying any UI Schema, and the CSS selectors is not a good option either, because it requires me to write extra CSS and have duplicate CSS and have to maintain that. I just want to use my existing CSS classes from Bootstrap.

@davewwww
Copy link
Contributor

davewwww commented Feb 9, 2024

@sdirix is changing the control.input in interface Styles from string to object an option?

export interface Styles {
  control: {
    root?: string;
    wrapper?: string;
    input?: {
      default?:string,
      number?:string,
      checkbox?:string,
    }
    textarea?: string;
    select?: string;
  }
}

i mean, it breaks the whole Styles Interface "key.key=value" structure.

an alternative could be:

export interface Styles {
  control: {
    root?: string;
    wrapper?: string;
    input?: string;
    inputNumber?: string;
    inputCheckbox?: string;
    textarea?: string;
    select?: string;
  }
}

and at the renderer:

    <input
      type="number"
      :class="[styles.control.input, styles.control.inputNumber]"
...

@sdirix
Copy link
Member

sdirix commented Feb 12, 2024

@davewwww All of the options are possible, we should just use the one which is most generic.

@lucas-koehler and me discussed this. We think a root level split would be best, e.g. something like this

  control: {
    checkbox: {
      root: 'control',
      wrapper: 'wrapper',
      label: 'label',
      description: 'description',
      error: 'error',
      input: 'input'
    },
    text: {
      root: 'control',
      wrapper: 'wrapper',
      label: 'label',
      description: 'description',
      error: 'error',
      input: 'input'
    },
  },

To avoid to redeclare the same properties for all splits, we could have a base or default entry which is used if there is not a more specialized property given.

Does this fit your needs?

@davewwww
Copy link
Contributor

Looks a bit overkill to me. i thought it was only about setting new classes at the input element based on its type?
what is the actual purpose of adding new css classes within the renderer from the outside?
my understanding of the vanilla renderer is that it is as basic as possible and that you can change the design from the outside via css query selector, like:

 .wrapper:has(input:where([type="number"])) {
    padding:1rem;
}

i see a need for adding a type specific class at the input elements, but why on root, wrapper, description, error, eg. that it needs a root level split? This Styles feature is currently still undocumented, so nothing is set in stone yet. But with this change, it would no longer be backwards compatible.

I would prefer a solution that keeps the "key.key=value" structure and adds another class to the input.

<input type="number" class="control control-type-number" />

@sdirix
Copy link
Member

sdirix commented Feb 13, 2024

Hi @davewwww,

Generally there are two different ways of styling the Vanilla renderers:

  • Writing custom CSS
  • Applying some CSS framework, e.g. Tailwind or Bootstrap

For writing custom CSS it's important that each rendered HTML element can be easily targeted. This is why we want to apply at least one class to all rendered elements.
Of course not all of these classes need to be different per input-type or control-type per default. As you have shown, one can for example write sophisticated CSS selectors to target the same class depending on the input with different rules. Still having the option to output a different CSS class based on the use case might be beneficial, e.g. less hard coded CSS selectors.

When using a CSS framework, the typical use case is to apply specific CSS classes to each rendered element. To support this use case JSON Forms needs to allow to apply arbitrary CSS classes to each element.

The original request in this issue is tailored to the input use case and suggests allowing applying different classes to inputs. However thinking from a framework perspective, I would expect that there is a future feature request allowing to add different classes to the wrappers of these inputs too.

Therefore our suggestion is to support this feature request generically and instead of only allowing to customize the classes of inputs, allow to customize the classes of all elements based on their type. This obviously solves the original request and allows other use cases too.

The current styles are already nested one level. Besides backward compatibility I don't see the benefit of keeping it to one level. Note that there is actually some documentation.

To make it a non breaking change we could use control as the base element, as it is right now, and use radioControl, checkboxControl etc. as siblings. The inheritance nature and scope is then only "visible" via the naming.

@davewwww
Copy link
Contributor

davewwww commented Feb 13, 2024

thanks for the explanation.
while working with tailwind i use the @apply feature a lot. so it is not necessary to inject their classes into the styles

so you mean something like that:

  1. as optional combined classes
<input :class="[styles.control.input, styles?.['numberControl']?.input]" type="number" />
  1. or only one class and the typed control class will overwrite the default class?
<input :class="styles?.['numberControl']?.input ?? styles.control.input" type="number" />

One more note: While working on my json forms editor, I need a JSON schema for the uischema.
With the current solution ("key.key=value") it is quite easy to have a generic validation for the styles. Since I also use a lot of custom renderers, it is a good behavior to follow this "key.key=value" structure as well. it will probably be more difficult if there is another way for defining styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants