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

Redesign FormLayout component #2201

Closed
KremnevDmitry opened this issue Sep 4, 2023 · 2 comments · Fixed by #2203
Closed

Redesign FormLayout component #2201

KremnevDmitry opened this issue Sep 4, 2023 · 2 comments · Fixed by #2203
Assignees
Labels
breaking changes Fix brings breaking changes in code or behavior in: flowui size: M
Milestone

Comments

@KremnevDmitry
Copy link
Contributor

KremnevDmitry commented Sep 4, 2023

How it worked in 2.0

The FormLayout component has two methods for adding internal components: add(Component), addFormItem(Component, Label).

When added via add: components will be added as is.
Adding via addFormItem allows you to display the label ASIDE.

The FormLayout component has a labelPosition attribute.
The formLayoytLoader works in such a way that if labelPosition attribute is specified explicitly, then the addFormItem method is used, otherwise use add method.

Why did we decide to change this?

This caused the following issues: #1989
If you use responsiveSteps with different labelPositions, they will be ignored, because components were added via the add method.
Let me remind you: labelPosition=ASIDE only works with wrapped formItem components.

In #1989 a wrapper of all components in formItem was implemented so that labelPosition = ASIDE would always work.

Why did we decide to change this again?

This caused the following problems: #2186
Another problem: changing the visible attribute for a component does not change its for their formItem.
Thus, there may be a case where the user has hidden a component, but its label remains on the view because the label is part of the formItem.

FormItem reserves space for labels, even if they are null.
So always using a wrapper is a bad idea.

It was decided to explicitly indicate the formItem in the XML markup if it is needed.
Thus, we will not hide the features of the implementation from users.
Also, Java API and XML will be consistent.

Please note: in order to hide a component with its FormItem, you must use the util method: io.jmix.flowui.kit.component.ComponentUtils#setVisible

Here are some examples of how it will work now

Case 1 (probably the most commonly used) [No BreakingChanges]

        <formLayout id="form" dataContainer="userDc">
            <textField id="usernameField" property="username" readOnly="true"/>
            <passwordField id="passwordField"
                           label="msg://com.company.formlayouttest.entity/User.password"
                           required="true"
                           visible="false"/>
            <passwordField id="confirmPasswordField"
                           label="msg://confirmPassword"
                           required="true"
                           visible="false"/>
            <textField id="firstNameField" property="firstName"/>
            <textField id="lastNameField" property="lastName"/>
            <textField id="emailField" property="email"/>
            <comboBox id="timeZoneField" property="timeZoneId"/>
            <checkbox id="activeField" property="active"/>
        </formLayout>

In this case, the components will be added via the add method and labelPosition = TOP as default value.
If users want to hide a component, or disable it, they can do so in the XML or through the controller by injecting the component directly.
Everything will work transparently.

Case 2 [Has BreakingChanges]

       <formLayout id="form" dataContainer="userDc" labelsPosition="ASIDE">
           <formItem id="usernameFormItem">
               <textField id="usernameField" property="username" readOnly="true"/>
           </formItem>
           <formItem id="passwordFormItem" label="msg://com.company.formlayouttest.entity/User.password" visible="false">
               <passwordField id="passwordField"
                              required="true"/>
           </formItem>
           <formItem id="confirmPasswordFormItem" label="msg://confirmPassword" visible="false">
               <passwordField id="confirmPasswordField"
                              required="true"/>
           </formItem>
           <formItem id="firstNameFormItem">
               <textField id="firstNameField" property="firstName"/>
           </formItem>
           <formItem id="lastNameFormItem">
               <textField id="lastNameField" property="lastName"/>
           </formItem>
           <formItem id="emailFormItem">
               <textField id="emailField" property="email"/>
           </formItem>
           <formItem id="timeZoneFormItem">
               <comboBox id="timeZoneField" property="timeZoneId"/>
           </formItem>
           <formItem id="activeFormItem">
               <checkbox id="activeField" property="active"/>
           </formItem>
       </formLayout>

By specifying labelPosition=ASIDE, users will have to wrap the components in a formItem themselves.
This way users will know when they are hiding a component and when they are hiding a FormItem.

Case 3 (ResponsiveSteps with TOP labelPosition) [No BreakingChanges]

       <formLayout id="form" dataContainer="userDc">
           <responsiveSteps>
               <responsiveStep minWidth="0" columns="1"/>
               <responsiveStep minWidth="20em" columns="2"/>
               <responsiveStep minWidth="65em" columns="4"/>
           </responsiveSteps>
           
           <textField id="usernameField" property="username" readOnly="true"/>
           <passwordField id="passwordField"
                          label="msg://com.company.formlayouttest.entity/User.password"
                          required="true"
                          visible="false"/>
           <passwordField id="confirmPasswordField"
                          label="msg://confirmPassword"
                          required="true"
                          visible="false"/>
           <textField id="firstNameField" property="firstName"/>
           <textField id="lastNameField" property="lastName"/>
           <textField id="emailField" property="email"/>
           <comboBox id="timeZoneField" property="timeZoneId"/>
           <checkbox id="activeField" property="active"/>
       </formLayout>

In this case, responsiveSteps will work correctly, because the formLayout will not have to change the labelPosition attribute depending on the current responsiveStep.

Case 4 (ResponsiveSteps with ASIDE labelPosition) [Has BreakingChanges]

        <formLayout id="form" dataContainer="userDc" labelsPosition="ASIDE">
            <responsiveSteps>
                <responsiveStep minWidth="0" columns="1"/>
                <responsiveStep minWidth="20em" columns="2"/>
                <responsiveStep minWidth="65em" columns="4"/>
            </responsiveSteps>
            
            <formItem id="usernameFormItem">
                <textField id="usernameField" property="username" readOnly="true"/>
            </formItem>
            <formItem id="passwordFormItem" label="msg://com.company.formlayouttest.entity/User.password" visible="false">
                <passwordField id="passwordField"
                               required="true"/>
            </formItem>
            <formItem id="confirmPasswordFormItem" label="msg://confirmPassword" visible="false">
                <passwordField id="confirmPasswordField"
                               required="true"/>
            </formItem>
            <formItem id="firstNameFormItem">
                <textField id="firstNameField" property="firstName"/>
            </formItem>
            <formItem id="lastNameFormItem">
                <textField id="lastNameField" property="lastName"/>
            </formItem>
            <formItem id="emailFormItem">
                <textField id="emailField" property="email"/>
            </formItem>
            <formItem id="timeZoneFormItem">
                <comboBox id="timeZoneField" property="timeZoneId"/>
            </formItem>
            <formItem id="activeFormItem">
                <checkbox id="activeField" property="active"/>
            </formItem>
        </formLayout>

In this case, the user is implying that components should always have a labelPosition=ASIDE.
You need to wrap the components in a FormItem for it.

Case 5 [Has BreakingChanges]

 <formLayout id="form" dataContainer="userDc">
            <responsiveSteps>
                <responsiveStep minWidth="0" columns="1" labelsPosition="TOP"/>
                <responsiveStep minWidth="20em" columns="1" labelsPosition="ASIDE"/>
                <responsiveStep minWidth="30em" columns="2" labelsPosition="TOP"/>
                <responsiveStep minWidth="65em" columns="2" labelsPosition="ASIDE"/>
            </responsiveSteps>

            <formItem id="usernameFormItem">
                <textField id="usernameField" property="username" readOnly="true"/>
            </formItem>
            <formItem id="passwordFormItem" label="msg://com.company.formlayouttest.entity/User.password" visible="false">
                <passwordField id="passwordField"
                               required="true"/>
            </formItem>
            <formItem id="confirmPasswordFormItem" label="msg://confirmPassword" visible="false">
                <passwordField id="confirmPasswordField"
                               required="true"/>
            </formItem>
            <formItem id="firstNameFormItem">
                <textField id="firstNameField" property="firstName"/>
            </formItem>
            <formItem id="lastNameFormItem">
                <textField id="lastNameField" property="lastName"/>
            </formItem>
            <formItem id="emailFormItem">
                <textField id="emailField" property="email"/>
            </formItem>
            <formItem id="timeZoneFormItem">
                <comboBox id="timeZoneField" property="timeZoneId"/>
            </formItem>
            <formItem id="activeFormItem">
                <checkbox id="activeField" property="active"/>
            </formItem>
        </formLayout>

In this case, the components need to be wrapped in a FormItem because the FormItem is responsible for displaying the label on the TOP and ASIDE.

Thus BREAKING CHANGES only if any labelPosition=ASIDE settings were used

@KremnevDmitry KremnevDmitry added breaking changes Fix brings breaking changes in code or behavior in: flowui labels Sep 4, 2023
@KremnevDmitry KremnevDmitry self-assigned this Sep 4, 2023
@KremnevDmitry KremnevDmitry changed the title FormLayout redesign Redesign FormLayout component Sep 4, 2023
@KremnevDmitry
Copy link
Contributor Author

KremnevDmitry commented Sep 5, 2023

  • layout.xsd [new FormItem element]
  • FormLayoutLoader rework, new FormItemLoader
  • Studio element annotation
  • Tests
  • Update project templates

Studio ticket: https://youtrack.jmix.io/issue/JST-4259/Support-redesign-FormLayout-component

@konyashkina
Copy link

Tested all described cases on
Jmix version: 2.1.999-SNAPSHOT
Jmix Studio plugin version: 2.1.SNAPSHOT5636-232
IntelliJ version: IntelliJ IDEA 2023.2.2 (Ultimate Edition)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Fix brings breaking changes in code or behavior in: flowui size: M
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants