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

FromControl label should have "for" attribute #445

Closed
Pitel opened this issue Jun 15, 2021 · 6 comments · Fixed by #452
Closed

FromControl label should have "for" attribute #445

Pitel opened this issue Jun 15, 2021 · 6 comments · Fixed by #452
Labels
api breaking Forces client sources to change bug Something isn't working

Comments

@Pitel
Copy link

Pitel commented Jun 15, 2021

Describe the bug
FromControl label should have "for" attribute, so when user click on the label, the input gets automaticaly focused.

To Reproduce
Steps to reproduce the behavior:

  1. Create simple formControl with label and inputField.
  2. Click on the label.
  3. Nothing happens.

Expected behavior
Cursor will appear inside the inputField (or, in case of chackbox, the checbox is toggled)

Additional context
Or, you can nest the <input>inside` to get the same behaviour.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label

@ghost ghost self-assigned this Jun 15, 2021
@ghost ghost added this to To do in Sprint 0.11.1 via automation Jun 15, 2021
@ghost ghost added the bug Something isn't working label Jun 15, 2021
@ghost ghost added this to the 0.11.1 milestone Jun 15, 2021
@ghost
Copy link

ghost commented Jun 15, 2021

Good catch - we will repair this soon, as we are working on some improvements for the formControl concept currently. So I will integrate this issue when solving #440 issue.

@ghost ghost moved this from To do to In progress in Sprint 0.11.1 Jun 18, 2021
@ghost ghost added the api breaking Forces client sources to change label Jun 18, 2021
@ghost
Copy link

ghost commented Jun 18, 2021

Ok, this is going to be harder than expected... (so it won't make it into the 0.11.1 version)

In order to make this work, we have to determine the ID of the control prior to its rendering. Thus the wrapped factory methods within FormControlComponent are the place to do that:

// get or cerate an ID
protected fun determinedControlId(external: String?, storeId: String?) =
    external ?: (storeId ?: "control-${uniqueId()}")

open fun inputField(
    styling: BasicParams.() -> Unit = {},
    value: Store<String>? = null,
    baseClass: StyleClass = StyleClass.None,
    id: String? = null,
    prefix: String = ControlNames.inputField,
    build: InputFieldComponent.() -> Unit = {}
) {
    val validationMessagesBuilder = ValidationResult.builderOf(this, value)
    // add new ``controlId`` parameter as new second param
    registerControl(
        ControlNames.inputField,
        determinedControlId(id, value?.id), // add determined ID
        {
            inputField(styling, value, baseClass, id, prefix) {
                size { this@FormControlComponent.sizeBuilder(this) }
                severity(validationMessagesBuilder().hasSeverity)
                build()
            }
        },
        { this.validationMessagesBuilder = validationMessagesBuilder }
    )
}

So the registerControl method needs also extension:

protected fun registerControl(
    controlName: String,
    controlId: String, // new
    component: (RenderContext.() -> Unit),
    onSuccess: FormControlComponent.() -> Unit = {}
) {
    // pass this to control -> make Triple out of Tuple (or custom data class?!)
    if (control.set(controlName, controlId, component)) {
        onSuccess(this)
    }
}

Make control public so that a ControlRenderer can access it and thus the ID:

    val control = Control()

Now we can access the ID for the control from a renderer:

class SingleControlRenderer(private val component: FormControlComponent) : ControlRenderer {
    override fun render(
        styling: BoxParams.() -> Unit,
        baseClass: StyleClass,
        id: String?,
        prefix: String,
        renderContext: RenderContext,
        control: RenderContext.() -> Unit
    ) {
        renderContext.stackUp(
            {
                alignItems { start }
                width { full }
                component.ownSize()()
                styling(this as BoxParams)
            },
            baseClass = baseClass,
            id = id,
            prefix = prefix
        ) {
            spacing { tiny }
            items {
                label({
                    component.labelStyle.value()
                }) {
                    // here we can finally access the desired ID
                    component.control.assignee?.let { `for`(it.third) }
                    className(formGroupElementLabelMarker)
                    +component.label.value
                }
                stackUp({
                    alignItems { start }
                    width { full }
                }) {
                    spacing { none }
                    items {
                        control(this)
                        component.renderHelperText(this)
                        component.renderValidationMessages(this)
                    }
                }
            }
        }.apply {
            className(formGroupElementContainerMarker)
        }
    }
}

This all sounds reasonable, but: How do we pass this ID down to the control itself!?

There are several solutions to this problem:

  • we hijack the id parameter of the control factory method → the stand alone component would loose the ability to pass an ID to the surrounding element!
  • we introduce a new parameter to all those components

This is not trivial to decide, so we must discuss the further approach within a bigger group!

Sidestep

We should nevertheless and idenpendently from the above feature pass a well defined ID as ID of the control in order to make it plausible for testing to observe a specific control element.

@ghost ghost removed this from the 0.11.1 milestone Jun 18, 2021
@ghost ghost removed this from In progress in Sprint 0.11.1 Jun 18, 2021
@Pitel
Copy link
Author

Pitel commented Jun 21, 2021

Why not just nest the <input> inside <label>? Then, you don't have to mess with ids at all.

@ghost
Copy link

ghost commented Jun 21, 2021

Because of #440 ;-)

We absolutly need two DOM elements on the same level to achieve this. So nesting is no option here.

Edit:
And on top this is due to the fact, that the formControl just add labels to any other form element. So those do no necessarily only consist exactly on one input element alone (take the switch for example).

@ghost
Copy link

ghost commented Jun 21, 2021

The problems mentioned in #445 (comment) aren't valid:

There are several solutions to this problem:

  • we hijack the id parameter of the control factory method → the stand alone component would loose the ability to pass an ID to the surrounding element! Not true! The surrounding element gets its id from the formControl factory function! So this is exactly the solution: Use the id from the different forms wrapped factory methods.
  • we introduce a new parameter to all those components -> obsolete

We should nevertheless and idenpendently from the above feature pass a well defined ID as ID of the control in order to make it plausible for testing to observe a specific control element.

This is quite easy though in combination with the above solution: Juts define a default value for id parameter != null!:

fun RenderContext.inputField(
    styling: BasicParams.() -> Unit = {},
    value: Store<String>? = null,
    baseClass: StyleClass = StyleClass.None,
    id: String = value?.id ?: "inputField-${uniqueId()}", // take the store's id if present, if not generate one.
    prefix: String = "inputField",
    build: InputFieldComponent.() -> Unit = {}
) {
    InputFieldComponent(value).apply(build).render(this, styling, baseClass, id, prefix)
}

Trouble

The above solutions work well - if the RootStore has got a unique ID! The default is an empty string -> this will lead to trouble, as there might be multiple euqal IDs within the DOM...

We should consider to apply always a random unique ID per default:

open class RootStore<T>(
    initialData: T,
    override val id: String = "" // This is the problem! ``uniqueId()`` to the rescue?
) : Store<T> {
    // and so on
}

@ghost
Copy link

ghost commented Jun 21, 2021

storeOf factory also needs the default ID uniqueId()!

ghost pushed a commit that referenced this issue Jun 21, 2021
- fixes #445
- formControl now determines an ID for each control and makes this available to the renderers, so they can use it as reference for semantically grouped elements like label's ``for`` and some related ``input``
- add default none empty ID to ``RootStore`` and its factory function ``storeOf``.
@ghost ghost mentioned this issue Jun 21, 2021
@ghost ghost added this to Review in progress in Sprint 0.11.1 Jun 21, 2021
@ghost ghost moved this from Review in progress to In progress in Sprint 0.11.1 Jun 22, 2021
@ghost ghost moved this from In progress to Review in progress in Sprint 0.11.1 Jun 22, 2021
@ghost ghost moved this from Review in progress to Reviewer approved in Sprint 0.11.1 Jun 22, 2021
@ghost ghost moved this from Reviewer approved to Done in Sprint 0.11.1 Jun 22, 2021
@ghost ghost added the component/styling label Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api breaking Forces client sources to change bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant