search

dhis2 / ui-forms

:no_entry: [DEPRECATED] Please refer to https://github.com/dhis2/ui
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Add defaultProps to `SingleSelect` and `MultiSelect` so there are default text available for components #211

Open HendrikThePendric opened 4 years ago

HendrikThePendric commented 4 years ago

From what I could tell, both the MultiSelect and the SingleSelect need defaultProps set for the same props, which are listed below.

clearText (Clear)
filterPlaceholder (Filter)
loadingText (Loading)
noMatchText (No results found)
placeholder (Please select an option / Please select one or more options)

@ismay could you check:

ismay commented 4 years ago

Probably good to get feedback from @varl on this as well. I remember us discussing where to have translations for the Select, but did we settle on how to approach it in the end? Would ui-widgets and ui-forms be appropriate for our translations for example? Also I'm not sure if there's anything in the works for the app shell and how it would interact with translations.

As for the Select properties: yes, those are the ones. Though I can also imagine that people wouldn't want them set. Maybe we should just pass them along somehow?

HendrikThePendric commented 4 years ago

but did we settle on how to approach it in the end?

Yes ui-forms and ui-widgets are the appropriate places to add translatable texts.

Though I can also imagine that people wouldn't want them set

My knowledge of the *Select may be too limited... It wasn't my intention to set default values for textual props that make something appear. I.e. clearText won't do anything unless clearable is also true, so in this case providing a default value is fine, right? If a different text is needed, the app-developer can just specify a clearText prop and that will be overridden.....

Looking at the *Select code, I can see that the placeholder will cause this text to show up unconditionally, so I guess we'd better not provide a default value for that.

@ismay could you make an educated guess as to which textual props should/ shouldn't get default values?

ismay commented 4 years ago

If we only want to set default text for text that requires a feature to be toggled to be displayed these are the texts you want:

I'm thinking though, maybe it's good to house all the translations in ui-widgets for example? Otherwise it might be awkward that we have form components translated here and in ui-widgets. Maybe someone wants to use translated form components, but not FF for example. We could then import the translated components from ui-widgets to ui-forms.

HendrikThePendric commented 4 years ago

@ismay thanks for that list of props that should get translated strings.

Regarding your suggestion for creating a ui-widgets component that holds translations: I'm not sure about this. On the one hand I think your line of reasoning is perfectly valid and thus would justify creating a component in ui-widgets. But on the other hand, I fear that then having three virtually identical components in three different repos could possibly cause more awkwardness than the amount of awkwardness we are trying to avoid.

I do seem to remember that at some point we agreed that the places to add translatable strings to components would be ui-widgets and ui-forms. If we head into the direction you're suggesting, then I think we might as well remove i18n from all the components in ui-forms (we'd still need it for the validators).

@varl @Mohammer5 Please share your views...

varl commented 4 years ago

If the direction is that ui-forms is a "headless" library and just wraps components from ui-core and ui-widgets, I can see the UI translations in widgets.

HendrikThePendric commented 4 years ago

If the direction is that ui-forms is a "headless" library

Yeah.... It could make sense to head into that direction (pun unintentional) but to my understanding that wasn't the original approach we agreed on....

I do seem to remember that at some point we agreed that the places to add translatable strings to components would be ui-widgets and ui-forms

Having said that, I've reviewed the code-base and the only other component that currently holds translations is the FileInput. And there is already an issue that requests moving the FileInput to ui-core https://github.com/dhis2/ui-core/issues/651. In my mind, this issue could then be replaced by an issue in ui-widgets titled Move FileInput from ui-forms to ui-widgets. So going the headless way would not be a big job.

BUT, even though it could make sense to move all the translations out of ui-forms on a conceptual level, I do still think it makes things really confusing for the app-developer. If we use the MultiSelect as an example, the app-dev will need to be aware of the following:

And just to compare that to the choice the dev would have to make if we would not introduce these components to ui-widgets purely for translations (i.e. keep having translations in ui-forms components):

Obviously this is a much clearer distinction, and would require the app-dev to maintain a much simpler mental model about how our repos relate to one another...

So in mind we are weighing up pros and cons again. Lets try to "quantify" them for the MultiSelect:

What do you think @varl @Mohammer5 @ismay... Go "headless" or not?

HendrikThePendric commented 4 years ago

After re-reading what I wrote above, I feel like I have neglected to mention the fact that in all likelihood the app-dev will be ui-widgets components already when building an app. So he/she will already need to have some working mental model in place of what distinguishes a ui-core component from a ui-widgets component.

So my comment above might have a bit of an unfair bias towards the "keeping i18n in ui-forms" solution.

Also, good documentation can go a long way here. We could even list the differences explicitly in all repos...

ismay commented 4 years ago

Yeah.... It could make sense to head into that direction (pun unintentional) but to my understanding that wasn't the original approach we agreed on....

The way I see our intention with this library is that we want it to be a transparent (or as transparent as possible) layer between our components and react final form. So to me, the scope of this library is very minor: a small shim that makes our components conform to the api that a react-final-form field expects (combined with some transform and validation helpers).

That keeps it maintainable for us, and understandable for devs using this lib as they're just using RFF in the end. I think adding translations to this lib stretches the scope of this lib somewhat, that's why I'd prefer not doing that.

BUT, even though it could make sense to move all the translations out of ui-forms on a conceptual level, I do still think it makes things really confusing for the app-developer.

To me the distinction makes sense if you look at it like this:

Since ui-forms is meant for convenience I'd default to the translated components from ui-widgets there. The dev will then still be able to override the translations where necessary by setting the props they want to override on the Field. That keeps all the translations in one place, and scoping-wise seems clear to me as well. What do you think @HendrikThePendric?

HendrikThePendric commented 4 years ago

What do you think @HendrikThePendric?

I am warming to the idea, but I think this is actually a fairly substantial decision to make, so I'd like to get some input from at least one other team member before we proceed with this.

varl commented 4 years ago

Going headless to me would mean that ui-forms would not have a dependency to ui-core/-widgets at all. Do we share that definition?

Thinking on this some more, I think that ui-forms being headless is the "right" way to go here:

  1. It makes the separation of concerns between ui-core / -widgets / -forms very clear.

  2. ui-forms could be used in situations where a hybrid approach is needed (e.g. you have some custom components, some core components, and some MUI components), which is the case for almost every application in existence.

  3. ui-forms hasn't been released yet, so we can still do this without breaking the world.

Question I don't know the answer to: how would a headless ui-forms library look in practice?

ismay commented 4 years ago

@varl I'm not sure what you mean by headless, do you mean that this lib should be able to wire up not just our components to RFF, but also MUI and custom components?

varl commented 4 years ago

@ismay "headless" to me means that it doesn't render anything by itself, so yes.

This is mostly to sort out the terminology we use so we all share the same understanding:

E.g.

<Input
  component={InputField}
  onChange={}
  onFocus={}
  ...props
/>

We'd tailor it to fit our API, but if you want to use a MUI field instead, you'd just have to pass in a callback to the handlers with the signature that MUI components expect (event instead of obj, event).

HendrikThePendric commented 4 years ago

I like the notion of being a headless lib, but as I see it some things stand in our way:

So, as a general approach, I like the headless suggestion, I just don't really see how we'd do it. Perhaps someone else does...

If you look at the code in the ./src/components folder, you will see that all components contain very little logic (except FileInput, but ignore that one, it will be moved to widgets/core): we're just mapping some props and computing a few of them via helpers. So making a MUI-based text input component would be equally easy for an app-dev. Ultimately, we want to offer a comprehensive set of components, all using the DHIS2 design system, so the app-dev won't have to take that step anymore. But until then, it's pretty easy to build one in the app, especially if we were to expose our helper functions (e.g. hasError, isValid etc.).

I guess what I am trying to say is, atm we are not headless, but our components are just a very thin layer between the RFF Field and the ui-core components. I think that's not bad either.

Mohammer5 commented 4 years ago

I have to agree with @HendrikThePendric, I don't really see a way to turn ui-forms into a headless library, as that's essentially what RFF already does.

I like the idea of having all translations (and therefore a translated component for each input component in ui-core) in ui-widgets.

Right now ui-core is a peer dependency of ui-widgets, so a non-breaking change in ui-core could be used in apps right away once that change has been released, same goes for ui-forms, which currently has ui-core as peer dependency, but once changed to ui-widgets, any non-breaking change in either ui-core or ui-widgets could be used right away once the change has been released.

Maybe we could give the ui-forms components a prop equal to the name of the component but pascal-case, which holds the component to be used and defaults to the ui-widgets component, here's an example:

import { CheckboxField } from '@dhis2/ui-widgets'

const Checkbox = ({
   // ... all existing props
   checkbox: Component,
}) => (
    <Component
        // ... all existing props
    />
)

Checkbox.defaultProps = {
    checkbox: CheckboxField,
}

This way an app could still use custom components which implement the same api as the ui-core components without having to do all the "magic" that's happening in ui-forms themselves (which eventually means that if that app moves away from RFF, the custom componenents' api still matches the ui-core components' apis.

varl commented 4 years ago

Sure. I'm bringing up the case now, since an immediate situation we saw with ui-core is that devs want to combine it with other components, so we need a way to build forms using a combination of "any components". If we do not easily have that ability then ui-forms practically cannot be used in "mixed apps" without dropping down to installing RFF as a direct dependency (no-go).

If RFF is already headless and can be used with any old component (makes sense), then maybe ui-forms needs to rethink its scope to stay that way, and expose things that RFF does not give us (validators, etc.)?

Mohammer5 commented 4 years ago

I get your point. Maybe we take ui-forms a bit apart and move things into appropriate domains. What I really like about the current implementation is that it's a bridge between ui-core and RFF, which is very nice to have because otherwise each app would have to implement this and 100% it would differ slightly from app to app, so this gives us some consistency.

If we change the scope of this to be a component-RFF-binding lib only, then we could move the validators to another library (not sure whether ui-core or ui-widgets are appropriate, but I'm also hesitant to create yet another library...)

ismay commented 4 years ago

What I really like about the current implementation is that it's a bridge between ui-core and RFF

Yeah it is convenient the way it currently works. And yeah, as mentioned already, I also think that if we want to be agnostic to the components used we can't stick with the current approach since it's completely tied to the api of our ui-core/widgets components.

If we want to decouple it a bit more I also think that a way to do that would be to provide more low-level helpers, that have to be wired up to a component in the consuming app. I do wonder how useful such a lib will be though. It's so low level that it's almost as much work as doing it yourself. Though I also get the intention to keep this lib generic. Keeping it generic might also prevent scope creep which is quite tempting with the current approach.

But it seems tricky to find a balance between a practical lib and a generic lib. Maybe we should implement it on a test branch in an app to see it in action. So we'll have a practical example to discuss.

HendrikThePendric commented 4 years ago

[...] devs want to combine it with other components, so we need a way to build forms using a combination of "any components". If we do not easily have that ability then ui-forms practically cannot be used in "mixed apps" without dropping down to installing RFF as a direct dependency (no-go).

Don't worry about that. This was one of the main things I had in mind when deciding on the current approach. I will prepare a small demo in a next comment.

If RFF is already headless and can be used with any old component (makes sense), then maybe ui-forms needs to rethink its scope to stay that way, and expose things that RFF does not give us (validators, etc.)?

I'm not sure about this actually. I was re-reading the presentation I prepared before I started working on this lib, and I feel like we've already hit the sweet spot when we initially defined the scope (mainly see slide 5 and also 2). What ui-forms will provide, and to a certain extend already does:

varl commented 4 years ago

Maybe we should implement it on a test branch in an app to see it in action. So we'll have a practical example to discuss.

We must do this before shipping the library as 1.0.0, it's not an optional step. We did that for ui core/widgets as well and the amount of learning we absorbed was significant.

Don't worry about that. This was one of the main things I had in mind when deciding on the current approach. I will prepare a small demo in a next comment.

Sounds good to me!

HendrikThePendric commented 4 years ago

Context

The app needs a form with some simple inputs, but also with a color-picker which we don't have in ui-core...

Code

STEP 1: Prepare a small wrapper component for your custom form input, which consumes the RFF fieldRenderProps API.

ColorPicker.js

import propTypes from '@dhis2/prop-types';
import { Field, getValidationText, hasError, Help, Label } from '@dhis2/ui-core';
import ColorPickerInput from 'rc-color-picker';
import React from 'react';

const ColorPicker = ({
    error,
    input,
    label,
    meta,
    validationText,
}) => (
    <Field>
        <Label required={required} disabled={disabled} htmlFor={input.name}>
            {label}
        </Label>

        <ColorPickerInput color={input.value}
            onChange={input.onChange}
        />

        {helpText && <Help>{helpText}</Help>}

        {hasError(meta, error) && (
            <Help error>
                {getValidationText(meta, validationText, error)}
            </Help>
        )}
    </Field>
)

ColorPicker.propTypes = {
    error: propTypes.bool,
    input: propTypes.obj,
    label: propTypes.string,
    meta: propTypes.obj,
    validationText: propTypes.string,
}

export const { ColorPicker }

STEP 2: use as normal

ColorForm.js

import React from 'react'
import { Button } from '@dhis2/ui-core'
import { Form, Field, Input, TextArea, hasValue } from '@dhis2/ui-forms'

import { ColorPicker } from './ColorPicker.js'

const onSubmit = values => {
    console.log('Do something with these form values', values)
}

const ColorForm = () => {
    <Form onSubmit={onSubmit}>
        {({ handleSubmit }) => (
            <form onSubmit={handleSubmit}>
                <h1>Create your own color</h1>

                <Field
                    name="name"
                    component={Input}
                    required
                    validate={hasValue}
                    label="Enter a name for your color"
                />

                <Field
                    name="description"
                    component={TextArea}
                    label="Describe your color"
                />

                <Field
                    name="colorCode"
                    component={ColorPicker}
                    required
                    validate={hasValue}
                    label="Select an actual color"
                />

                <Button primary type="submit">
                    Submit
                </Button>
            </form>
        )}
    </Form>
}

export { ColorForm }

So, the above example illustrates how to combine our ui-forms components (and validators) with something custom. The example is pretty basic, but does illustrate the steps that would be required in all cases. For example, in the user-app I have done exactly the same for some d2-ui components, although that was with Redux-form, but the principle remains the same.

HendrikThePendric commented 4 years ago

Maybe we should implement it on a test branch in an app to see it in action. So we'll have a practical example to discuss.

We must do this before shipping the library as 1.0.0, it's not an optional step. We did that for ui core/widgets as well and the amount of learning we absorbed was significant.

@varl Going back on to @ismay's original comment, I believe that "implement it" refers to trying to implement ui-forms as a headless lib. Is that also what you are referring to? Or do you mean that we should just work on some forms that have mixed fields (custom/MUI-based/d2-ui-based)?

ismay commented 4 years ago

@varl Going back on to @ismay's original comment, I believe that "implement it" refers to trying to implement ui-forms as a headless lib.

Not necessarily. I meant more to use the lib (in whichever form we decide to build it eventually) in an actual app. To get a feel for how it'd interact with one of our actual apps.

That way we can have some practical examples. Prevent the discussion from getting too theoretical/hypothetical.

Mohammer5 commented 4 years ago

I'd propose to create a branch on the import export app and try it there as it's structure currently fits ui-forms philosophy the most (although not 100%)

HendrikThePendric commented 4 years ago

OK sounds like a plan!

HendrikThePendric commented 4 years ago

I've had a quick look at the form elements in the import-export-app and I think it would be a good fit for the following reasons:

But there is also a downside:

I don't necessarily think that last point is a deal-breaker, because creating a form input from scratch isn't fundamentally different than creating a form input based on a d2-ui component.

varl commented 4 years ago

Nice, sounds like a solid plan.

HendrikThePendric commented 4 years ago

So... the discussion about going headless seems to have been resolved, and we are not going that way. We are keeping the current approach and are going to try this lib in the import-export-app before publishing v1.

But, keeping the current approach, the original discussion topic remains relevant and seems unresolved...

Should we keep translations in our components here, or introduce components in ui-widgets for this?

@varl @ismay @Mohammer5 please let me know.

Mohammer5 commented 4 years ago

I like the idea of having all translations (and therefore a translated component for each input component in ui-core) in ui-widgets.

Right now ui-core is a peer dependency of ui-widgets, so a non-breaking change in ui-core could be used in apps right away once that change has been released, same goes for ui-forms, which currently has ui-core as peer dependency, but once changed to ui-widgets, any non-breaking change in either ui-core or ui-widgets could be used right away once the change has been released.

I stick to this

ismay commented 4 years ago

But, keeping the current approach, the original discussion topic remains relevant and seems unresolved... Should we keep translations in our components here, or introduce components in ui-widgets for this?

I second @Mohammer5's preference. Keeping translations limited to ui-widgets seems most clear to me as well.

HendrikThePendric commented 4 years ago

OK. Well then I propose the following tasks need to be completed:

@Mohammer5 I think that as a side effect of this decision, it would also make sense to close https://github.com/dhis2/ui-core/issues/651 because app-devs are better of just using the FileInput from ui-widgets. Agreed?

HendrikThePendric commented 4 years ago

After discussing with @Mohammer5 the tasks have changed somewhat:

HendrikThePendric commented 4 years ago

See https://jira.dhis2.org/browse/TECH-288