Skip to content

[4.0][RFC] Remove 3 column layout and introduce fieldWidth attribute as layout hint#31810

Closed
HLeithner wants to merge 7 commits intojoomla:4.0-devfrom
HLeithner:j4/columns
Closed

[4.0][RFC] Remove 3 column layout and introduce fieldWidth attribute as layout hint#31810
HLeithner wants to merge 7 commits intojoomla:4.0-devfrom
HLeithner:j4/columns

Conversation

@HLeithner
Copy link
Member

Pull Request for Issue #25891 .

This is a RFC and all input is welcome also scss cleanups and tweaks would be helpful.
Thanks to @bembelimen for helping me.

Summary of Changes

Removed the automatic 3 and 2 column layout in backend formfield and introduced a hint attribute for form fields to let the template know about the expected width of the field.

The fieldWidth attribute is a abstract string value of the following (core supported) values:

  • full - 100%
  • large - 75%
  • medium - 50%
  • small - 25%
    Percent values are bs breakpoint xl values (getting bigger on smaller screens)

Examples:

  • default full
  • Module Style is large
  • Module Tag is medium
  • Bootstrap Size is small
  • header tag is small
  • header class is full

2k Screen:
image

1200px:
image

922px:
image

770px:
image

707px:
image

Testing Instructions

Formlayouts in backend

Actual result BEFORE applying this Pull Request

3 or 2 columns

Expected result AFTER applying this Pull Request

one column

Documentation Changes Required

yes new attribute for form field

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev RFC Request for Comment labels Dec 30, 2020
@brianteeman
Copy link
Contributor

Thanks for this. As its an RFC here is a comment.

The 100% width on inputs is not viable. Specifically with any field that has a dropdown or an appended button. From a UX perspective they force you to move your eyes and your mouse to a completely different part of the screen. The wider the screen the worse the problem.

image

From an accessibility perspective they create the issue above and this is particularly bad for anyone using a screen magnifier or those people who struggle to keep visual focus as they move across the screen.

Both the UX and Accessibility issues are resolved with a maximum width for any field that has a dropdown or an appended button

@brianteeman
Copy link
Contributor

I did make a recording of the issue when using a screen magnifier but I can't find the post that it is in right now. There is also extensive research about the maximum width to use in order to avoid issues when you move your eyes from left to right and then back to the left without losing focus on the correct line. This has usually been done in relation to text but the principle applies. https://brian.teeman.net/web/846-length-matters

@chmst
Copy link
Contributor

chmst commented Dec 30, 2020

If I understand right, we can choose the width of the select box via a new fieldwidth attribute. This looks very promising.
Too late for testing now, but thank you for this PR, @HLeithner.

@dgrammatiko
Copy link
Contributor

@HLeithner @brianteeman this can be easily changed from width to max-width and instead of % some predefined 00rem could be set, so the forms should ALWAYS BE one column as this is the best practice but also the width will not exceed some logical boundaries (less zig-zag as Brian demonstrated)

@brianteeman
Copy link
Contributor

@dgrammatiko which is what I have been saying for about 2 years

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Dec 31, 2020

Actually I just read the code here and I'm totally against this. The width of the fields should be done in the parent div not on each individual field. Fields should be 100% (unless not feasible, eg switcher) and the parent div/fieldset should determine the total width. What this pr is doing is the same thing that joomla did with bootstrap 2. Which apart from being completely outdated is way too limiting for anyone that wants to do something custom...

Edit: to give a simple example why this never worked or will never work think that in order to override the classes on any individual field you need to write your own plugin (having the size controlled from the parent element xmls are irrelevant and possible for any frontender). Going this way it means that any frontender that has to work on joomla should be able to roll their own plugins and that's totally stupid...

@HLeithner
Copy link
Member Author

Ok, I will try to explain my motivation behind this proposal.

  • We have a 3/2 column auto layout, people are unhappy because it doesn't work for them. Removed it ✅
  • The developer has the need to tell the template how big the input is expected. This interfere with the paradigm "everything has to have the same width", because input fields are different. Brian image is nice but doesn't reflect reality in my opinion, you don't read a complete line but anyway. I created an example page to demonstrate how the width points (full, large, medium, small) could look like in atum.
  • For a better understanding for the need of developer "hints" for the template creator, the template can completely ignore this parameter but could work with it, also in the frontend. It solve the size problem in an abstract way so it's not bound to any framework or class or fixed size.
  • The width hint can be part of the form field it self and makes (useful or not) defaults for different fields
  • Field default examples (Maybe I don't get all field names right sorry for that):
    • JFormField (default for all fields) medium (not full as in the example/demonstration screenshots)
    • JFieldText medium
    • JFieldNumber small (typically a number doesn't need 300+ px) but only a suggest can be medium of course
    • JFieldList medium
    • JFieldEditor full
    • JFieldTextarea large or full
    • JFieldImage large so it can display the image?
    • JRepeatables full because it maybe has many fields?
    • JFieldRules full because it has many items
    • CustomFieldXYZ of 3rd party developer which has multiple fields with own labels or icons or whatever large
    • CustomFieldMap A openstreetmap Field which displays a map + some input fields full
    • Any many other fields I have no idea what other people can think of
  • The current (and older bs2 implementation) didn't allow the developer to tell the template how much width is actually needed for the field and the width may doesn't actually depend on the field it could also depend on the content that will be in this field (that's the reason for the XML approach) so you can adjust the field based on the used context. Example for JFieldTextarea, in one context it's used for 5 keywords in the other context it's a replacement for Editor Field. For 5 keywords I don't need 100% width but for the description it may makes sense.
  • Now with this proposal the template builder has a clue how big the field should be and can display it in a proper size.
  • The 3rd party developer that injects his fields with a plugin doesn't need to think about any painful javascript or css or classes to workaround a 250px width field if he/she needs it at maximum width.

That's my thought on this. Now your comments on your feedback.

Brian

The 100% width on inputs is not viable. Specifically with any field that has a dropdown or an appended button. From a UX perspective they force you to move your eyes and your mouse to a completely different part of the screen. The wider the screen the worse the problem.

From an accessibility perspective they create the issue above and this is particularly bad for anyone using a screen magnifier or those people who struggle to keep visual focus as they move across the screen.

Both the UX and Accessibility issues are resolved with a maximum width for any field that has a dropdown or an appended button

I think you didn't got that this was a demonstration how fields could look like and not how it should in the end. Giving all fields a width hint of "medium" would be a solution for your comment (Maybe I'm wrong). Beside that, I don't believe that you or anyone else checks always the complete line before he/she reads the next line, actually I would expect that you read the labels first and then the inputfield but maybe that's a wrong assumption (even if it's true for me). For the Dropdown the user doesn't have to click on the arrow but I think you know that. But again it's only an example and the first input fields aren't changed to any value.

Brian

I did make a recording of the issue when using a screen magnifier but I can't find the post that it is in right now. There is also extensive research about the maximum width to use in order to avoid issues when you move your eyes from left to right and then back to the left without losing focus on the correct line. This has usually been done in relation to text but the principle applies. https://brian.teeman.net/web/846-length-matters

Again it's not always about a simple textfield, a world map with only 300px (or any other fixed value) has a bad UX.

Dimitris
this can be easily changed from width to max-width and instead of % some predefined 00rem could be set, so the forms should ALWAYS BE one column as this is the best practice but also the width will not exceed some logical boundaries (less zig-zag as Brian demonstrated)

That's an implementation detail that I'm happy to improve if it fit's the proposal. Actually I'm not seeing why my implementation has more then one column?

Brian
which is what I have been saying for about 2 years

I really appreciate your input but I see no relevance for this comment for the discussion.

Dimitries
Actually I just read the code here and I'm totally against this. The width of the fields should be done in the parent div not on each individual field. Fields should be 100% (unless not feasible, eg switcher) and the parent div/fieldset should determine the total width. What this pr is doing is the same thing that joomla did with bootstrap 2. Which apart from being completely outdated is way too limiting for anyone that wants to do something custom...

I'm happy for any improvement you can introduce here, I actually don't see the limitation but yeah if you create a pr or a code sniped I can rework the implementation. If you have an Idea to solve this (in a simply way not to rewrite JformFields completely) it would be great, but you don't have to.

Dimitries
Edit: to give a simple example why this never worked or will never work think that in order to override the classes on any individual field you need to write your own plugin (having the size controlled from the parent element xmls are irrelevant and possible for any frontender). Going this way it means that any frontender that has to work on joomla should be able to roll their own plugins and that's totally stupid...

I can't follow you, the template doesn't need to implement the classes used in the layout, actually they can simple override it and do what they think is the best for the different width even if it's 100% for all. I see no plugin. You also can override the fieldWidth before rendering the field in the layout.

And calling a contribution totally stupid doesn't making it easier to contribute.

@dgrammatiko
Copy link
Contributor

And calling a contribution totally stupid doesn't making it easier to contribute.

I didn't call the contribution stupid, what I meant and I thought was clear by my comment was the falling back to the bootstrap 2 way of doing things and yes after 10 years using such outdated solutions is not very clever or future proofed.

You also can override the fieldWidth before rendering the field in the layout.

That means that you have to override the particular layout of each field (kinda ok) but what if you need to differentiate the same field which is used multiple times in a form (the point of individual classes per field)? My point is that the width should be controlled on the container not on each individual field. You're over engineering this for no obvious benefit (from my point of view this is harder to deal than a single class on the container of the fields ether the div or fieldset). Anyways this is just my opinion people don't really do overrides (which is sad as that's the platforms true way of doing things right) so that affects only the very few front enders that still deal with custom templates.

@HLeithner
Copy link
Member Author

I didn't call the contribution stupid, what I meant and I thought was clear by my comment was the falling back to the bootstrap 2 way of doing things and yes after 10 years using such outdated solutions is not very clever or future proofed.
ok

That means that you have to override the particular layout of each field (kinda ok) but what if you need to differentiate the same field which is used multiple times in a form (the point of individual classes per field)? My point is that the width should be controlled on the container not on each individual field. You're over engineering this for no obvious benefit (from my point of view this is harder to deal than a single class on the container of the fields ether the div or fieldset). Anyways this is just my opinion people don't really do overrides (which is sad as that's the platforms true way of doing things right) so that affects only the very few front enders that still deal with custom templates.

Maybe I don't understand what you mean but isn't that what I'm trying to solve?

I try to explain it with code.

You have a simple form.xml

<form>
	<fields name="params">
		<fieldset name="advanced">
			<field
				name="alias"
				type="text"
				label="A field with a short string"
				fieldWidth="small"
			/>

			<field
				name="title"
				type="text"
				label="The long title"
				fieldWidth="large"
			/>
		</fieldset>
	</fields>
</form>

This will produce something like the following html

<fieldset id="fieldset-advanced"><legend>Advanced</legend>
  <div>
    <div class="control-group small">
	<div class="control-label">
           <label>A field with a short string</label>
        </div>
	<div class="controls">
          <input type="text" name="alias" id="..." value="alias-from-the-title" class="form-control">
        </div>
    </div>
    <div class="control-group large">
	<div class="control-label">
           <label>A field with a short string</label>
        </div>
	<div class="controls">
          <input type="text" name="title" id="..." value="This is an awesome example with a long title so we need much space" class="form-control">
        </div>
    </div>
  </div>
</fieldset>

So any template implementing the class (at the moment small and large) can say small fields are only 200px width and large fields have the label on it's own row and the field has 350px.

I see no way to todo this if you give the class to the fieldset and it's already on the div for the field. Maybe you can give me an example?

@dgrammatiko
Copy link
Contributor

I see no way to todo this if you give the class to the fieldset and it's already on the div for the field. Maybe you can give me an example?

Pff, my bad, I'm sorry, I thought that the final output was already simplified eg:

    <div class="control-group">
           <label>A field with a short string</label>
          <input type="text" name="title" id="..." value="This is an awesome example with a long title so we need much space" class="form-control">
    </div>

but that's still not the case I guess

@HLeithner
Copy link
Member Author

Our field renderer uses this layout:

<div class="control-group<?php echo $class; ?>"<?php echo $rel; ?>>
<div class="control-label<?php echo $hide; ?>"><?php echo $label; ?></div>
<div class="controls">
<?php echo $input; ?>
<?php if (!$hideDescription && !empty($description)) : ?>
<div id="<?php echo $id; ?>">
<small class="form-text text-muted">
<?php echo $description; ?>
</small>
</div>
<?php endif; ?>
</div>
</div>

So we have some wrappers around the input.

Any suggestions to make this PR better or an alternative that fits better?

@dgrammatiko
Copy link
Contributor

Any suggestions to make this PR better or an alternative that fits better?

This is fine as is (I'm an idiot I should check the code before replying). I guess only a change to max-width with some logical values per breakpoint will also satisfy Brian's suggestion.

FWIW the wrapper div around label and input is totally superfluous, it just adds more DOM elements for no reason. Flex can deal perfectly without these wrappers. This is some legacy code that we can definitely drop in 2020/2021

@HLeithner
Copy link
Member Author

FWIW the wrapper div around label and input is totally superfluous, it just adds more DOM elements for no reason. Flex can deal perfectly without these wrappers. This is some legacy code that we can definitely drop in 2020/2021

That's maybe true for "simple" input fields but it's maybe hard for complex fields with multiple elements? And yes I got already told that this is bs2 by @bembelimen ;-) but for b/c reasons it shouldn't be a too big problem.

Anyway how could we improve the s/css? I'm really not an expert on this topic.

I'm also pretty sure that some more logic based on breakpoints or size or other parameter could make this more useful but it shouldn't get to complicated because overriding or implementing this in a custom template maybe too hard then.

@ceford
Copy link
Contributor

ceford commented Jan 2, 2021

I have tested this item ✅ successfully on 971d921

The field width attribute seems to work well - I always thought these full width fields were ugly and awkward to use.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31810.

@joomla joomla deleted a comment from ceford Jan 2, 2021
@HLeithner
Copy link
Member Author

I removed the relative values, and fixed the different sizes to
full 100%
large 670px
medium 470px (default)
small 220px

@brianteeman would have a look on the field sizes please?

@brianteeman
Copy link
Contributor

Thanks @HLeithner I will put this on my todo for Sunday/Monday

@brianteeman
Copy link
Contributor

Two quick comments

  1. You stated above that my example image was not relevant as no one reads the entire row. Thats exactly the point. When its a select you have to reach the far end of the row in order to find and hit the dropdown arrow. Setting a sensible max width resolves that accessibility problem

  2. Unless I am missing something every field with a size=xx, which is almost all, can have that line removed as it never does anything. The css width value in this PR (and before) overrides the size making it redundant code

@Quy
Copy link
Contributor

Quy commented Jan 3, 2021

When its a select you have to reach the far end of the row in order to find and hit the dropdown arrow.

Clicking anywhere on the select will display the dropdown.

@brianteeman
Copy link
Contributor

But you have to see the full width to know it's a select and not an input. Not easy if you are magnified

@HLeithner
Copy link
Member Author

The fields has a fix width now if they are too big they warp to the next function, if you think the width doesn't fit our needs we can talk about it.

Current widths:
full 100%
large 670px
medium 470px (default)
small 220px

you have to add 240px if you want the label on the same line. I'm open for other values. On thing to consider is the mix between medium and small, if you mix them they are aligned now, if you make small smaller it's not longer possible to use it for calendar fields. I didn't fine tuned large for this layout.

@brianteeman
Copy link
Contributor

Any reason that this is still a draft and didnt make it into the beta6

@HLeithner
Copy link
Member Author

I'm not sure about the attribute name (maybe hintWidth or widthHint is better), I have to remove the com_modules xml modifications and remove from all layouts the class="column-count-md-2 column-count-lg-3"

beside this I think it should work for most situations if @wilsonge is ok with it I will complete it

@ciar4n
Copy link
Contributor

ciar4n commented Feb 10, 2021

Only seeing this now. This is just a thought but you could possibly improve on this with CSS grid. Create a simple 4 column grid. Rather than setting a percentage, you can define how many columns a field spans. Items will then flow to fill the grid like in the following example...

image

You can also set a value that pushes a field to a new line if required. Eg the 'Linked Titles' field in the following example...

image

@ciar4n
Copy link
Contributor

ciar4n commented Feb 10, 2021

You can probably remove the associated column count CSS. I don't believe it is used anywhere else...

// Column Count
[class^="column-"],
[class*=" column-"] {
padding-bottom: 1rem;
column-gap: 2rem;
> div {
display: inline-flex;
flex-direction: column;
width: 100%;
break-inside: avoid;
page-break-inside: avoid;
&.hidden {
display: none;
}
.control-label {
width: 100%;
}
}
.form-no-columns & {
column-count: auto;
}
}
@include media-breakpoint-up(sm) {
.column-count-sm-2 {
column-count: 2;
}
.column-count-sm-3 {
column-count: 3;
}
}
@include media-breakpoint-up(md) {
.column-count-md-2 {
column-count: 2;
}
.column-count-md-3 {
column-count: 3;
}
}
@include media-breakpoint-up(lg) {
.column-count-lg-2 {
column-count: 2;
}
.column-count-lg-3 {
column-count: 3;
}
}

@brianteeman
Copy link
Contributor

@ciar4n one of the problems with the columns is with the display of popups. You can see it easily in action by clicking on all of the colour fields in the atum template style

@ciar4n
Copy link
Contributor

ciar4n commented Feb 11, 2021

@brianteeman You may misunderstand. I fully agree with the PR and the issues with columns. The default would still be a single column. I'm suggesting that the option of columns can be given. So instead of small, medium and large we could have...

span-1 (25% width - single column)
span-2 (50% width - single column)
span-3 (75% width - single column)
span-4 (default - full width - single column)

For the option of displaying fields inline / in a row we would have...

span-1-inline (25% width - inline)
span-2-inline (50% width - inline)
span-3-inline (75% width - inline)

Actually we could probably still use the same naming, just add small-inline, medium-inline and large-inline. I just got my grid cap on suggesting span-*

It's just a thought and maybe not required.

@ciar4n
Copy link
Contributor

ciar4n commented Feb 11, 2021

Also worth mentioning that what I'm suggesting can be applied after in a follow up PR.

@HLeithner
Copy link
Member Author

This PR is not for layouting a form, it's only for giving the designer a "hint" how big the form field should be.

@brianteeman
Copy link
Contributor

what is "removing 3 column layout" if not "layouting a form"

@wilsonge
Copy link
Contributor

I've merged #32422 which does a similar thing to this so closing this

@wilsonge wilsonge closed this Feb 17, 2021
@HLeithner
Copy link
Member Author

Awesome george, now we have again coupled the layout to the xml.

@wilsonge
Copy link
Contributor

By definition of having classes the layout is coupled to the xml...

You had a fieldWidth property. how is that not display coupled to the XML?

@HLeithner
Copy link
Member Author

the class attribute is directly used in the html, the fieldWidth is thought as a hint how big the content of the field is.

now you say class="span-1" and the template has to deal with it and has to create a class which can have span-1. Instead of having a hint how big a field is small medium large what ever. where the template makes a col-5 or maybe span-1 or anything else used in the css framework to fit the need.

additional to fieldWidth would be a fieldHeight or similar would make sense to give some more abstract help from the developer to the designer.

I would go much further on this with dependency and flow attributes but that was out of scope of this flow fix.

I personally would remove class and style attribute from the formfield completely because it's the wrong, the xml can't style a field. That's not the job of the field definition, it should only define the function not the layout.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 18, 2021

That's not the job of the field definition, it should only define the function not the layout.

💯 this. We keep coupling and imposing things to front end devs. The styling is something that the JLayouts should impose not the XMLs

Just to clarify the extra CSS from the other PR is fine but I would prefer if the application of those classes happened in the JLayouts and not in the XMLs

@ciar4n
Copy link
Contributor

ciar4n commented Feb 18, 2021

#32422 These classes are added to and a feature of the template. How that feature is applied is through a class attribute that already exists. There is nothing that is hard coupled. It is the same here except it's hidden behind a 'fieldWidth' attribute... you are still adding a class to a field via the XML.

As for the idyllic notion that field definition should not allow the addition of classes. Someone describe an as easy alternative? The definition can define markup and js... why not classes/css?

@HLeithner
Copy link
Member Author

You are right at this stage the pr does exactly the same as your PR with only one different I doesn't force the template build to use the same classes then me. Actually that wouldn't be a problem to use classes if they are defined by x for this and y for this.

But reality is that 3rd party devs add or use other classes in there notation which is basically fine but not if you try to use the same xml for j3 and j4 and j5. Because then you have to (or should) do something in this case keep a b/c layer for span-1. Also my solution doesn't allow you to say this field has to be inline, because this is a decision of the template designer and not of the developer same for offset. which is basically nothing different then what we have now with bootstrap classes.

Nothing against using our own classes for this, but then we should have our own css framework and pattern library and all that stuff so people know if you use Joomla you have to use the css framework or use another cms.

@ciar4n
Copy link
Contributor

ciar4n commented Feb 18, 2021

I doesn't force the template build to use the same classes then me.

😕 Yes you do, you are adding classes to the atum template css just like the other pr. If a template dev wants this same functionality, with the same class names, then, they to must add your corresponding css. And honestly I dont see the problem with that. Not all templates will want to support multi column or multi width fields and that is completely their choice. Only they can decide how their design can accommodate fields.

@Magnytu2
Copy link

Magnytu2 commented Mar 5, 2021

Hello, I have a quick question. I have tested all versions of Joomla! 4. I have just installed the latest night version. I'm wondering why you removed the 2 or 3 column display? The 1-column display is simply unusable on a computer screen.
Capture d’écran 2021-03-05 à 18 51 37

@HLeithner
Copy link
Member Author

@Magnytu2 as you may notice this PR didn't get merged, #32422 got merged a follow up pr seems is missing to clean up all fields to a proper layout.

@infograf768
Copy link
Member

@HLeithner
Could you suggest the solution for the screen above?
If I add the new classes to control-group, I can reduce the length of the input or select field, but not at all if I do for each field in the xml. Also these new classes don't seem to be useful to create multiple columns.

@HLeithner
Copy link
Member Author

I think @ciar4n is the person to tell you, a follow up patch from him with some examples would be great.

And talking about the implementation detail should be done in #32422 because if we use this it would confuse people later.

@infograf768
Copy link
Member

Ok, posted there.

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

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester RFC Request for Comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.