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

[4.0] Flex based control-group fields #27684

Merged
merged 15 commits into from
Jan 30, 2020

Conversation

ciar4n
Copy link
Contributor

@ciar4n ciar4n commented Jan 27, 2020

Pull Request for Issue #27558, #27681, #27676 #27664 .

Summary of Changes

Moves field control-groups to flex.

  • Less CSS required
  • Flex naturally supports RTL so no RTL CSS required.
  • Media queries look at the width of the viewport and are the only way to change the orientation of the current control-group fields. Flex would allow us to change the orientation of the field depending on the space available to it. A much more versatile and safer way to approach responsive fields especially.
  • Duplicate CSS removed
  • Fixes form-vertical class

Simply put, with this PR, label/fields will become container aware, moving from vertical to horizontal aligned if the field is reduced to less than 210px. I have also enabled the form-vertical class. The addition of this class and the field will always display vertically. The fields-no-margin has been removed completely.

Edit: Any field in a multi-column layout, displays in a vertical format regardless of the space available to it. This was always the case.

Testing Instructions

Apply this patch and run node build.js --compile-css for updating the changed SCSS. Check fields display correctly.

Documentation Changes Required

@Fedik
Copy link
Member

Fedik commented Jan 27, 2020

Less CSS required

I thought Joomla! 4 use scss 😋

@brianteeman
Copy link
Contributor

Might be me doing something wrong but something isnt right with the switcher now

Before

image

After

image

@joomla-cms-bot joomla-cms-bot removed NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jan 27, 2020
@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 28, 2020

@brianteeman Do you know of any reason why the switcher has a sr-only class on the label and then adds a second visible 'label' in the form of a legend within the field control? It seems strange and at odds with all other fields.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 28, 2020

Switcher html...

<div class="control-group">
  <div class="control-label sr-only">
    <label id="jform_featured-lbl" for="jform_featured">
      Featured
    </label>
  </div>
  <div class="controls">
    <fieldset id="jform_featured">
      <legend class="switcher__legend">
        Featured
      </legend>
      <div class="switcher">
        <input type="radio" id="jform_featured0" name="jform[featured]" value="0">    <label for="jform_featured0">No</label>         <input type="radio" id="jform_featured1" name="jform[featured]" value="1" checked="checked" class="active">   <label for="jform_featured1">Yes</label>    <span class="toggle-outside"><span class="toggle-inside"></span></span>
      </div>
    </fieldset> 
  </div>
</div>

@brianteeman
Copy link
Contributor

Yes. It was the only way to achieve the correct accessibility. But without checking the original pr I don't remember more than that

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 28, 2020

Does that mean only the switcher has the correct accessible html? If so then we can probably close this until all other fields are brought up to speed. Considering the switcher html, this PR would be quite different.

@brianteeman
Copy link
Contributor

switcher is a fieldset and a fieldset requires a legend

@C-Lodder
Copy link
Member

C-Lodder commented Jan 28, 2020

@ciar4n - The switcher is like this because you can't associate a label with multiple inputs...only a single input. The only other form fields I can think of that may need checking are checkbox/radio....but only when using multiple.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 28, 2020

Ok thank you. I'm guessing we can't just move the sr-only class from the label to the legend?

@C-Lodder
Copy link
Member

C-Lodder commented Jan 28, 2020

I think it's being used incorrectly so that the .control-label doesn't take up any space.

Instead the .sr-only should be moved to the <label> and a d-none class be added to the parent <div>

@brianteeman
Copy link
Contributor

give me two seconds to check that. my memory is failing me but there must have been a reason I did it that way

@brianteeman
Copy link
Contributor

changing the sr-only to being on the legend instead of the label seems to be ok. not sure I understand the d-none comment

@C-Lodder
Copy link
Member

C-Lodder commented Jan 28, 2020

@brianteeman Form fields in Joomla have a label to the left of the field (220px in width or somthing). This isn't the case with the switcher. Instead it's to the right hand side.
So to hide that 200px gap to the left of the switcher, they've used sr-only on <div class="control-label sr-only">.

@brianteeman
Copy link
Contributor

brianteeman commented Jan 28, 2020

my brain isnt in gear sorry.

The label to the right is the label for the value of the radio = eg yes/no
the field label and legend are to the left

why I chose to hide the label and not the the legend is something I dont remember and looking at the original pr doesnt help me

@brianteeman
Copy link
Contributor

corrected last comment

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jan 28, 2020
@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 28, 2020

I've moved the sr-only from the legend to the label which fixes the switcher style issue.

@Quy
Copy link
Contributor

Quy commented Jan 29, 2020

Looks good!! Thank you.

@infograf768
Copy link
Member

Anyone tested rtl ?

@Quy
Copy link
Contributor

Quy commented Jan 29, 2020

There is extra padding on the first field.

27684-top

@Quy
Copy link
Contributor

Quy commented Jan 29, 2020

Anyone tested rtl ?

Yes, looks fine.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 29, 2020

There is extra padding on the first field.

@Quy Top margin removed.

@Quy
Copy link
Contributor

Quy commented Jan 29, 2020

I have tested this item ✅ successfully on e476392


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

@infograf768
Copy link
Member

Works fine here but one question:

@ciar4n
Is it possible to also correct the Global Configuration Text Filters tab in reduced window width (LTR and RTL)? Or shall we postpone this to another PR (if even possible)?

Screen Shot 2020-01-30 at 08 35 46

Screen Shot 2020-01-30 at 08 36 43

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 30, 2020

Outside the scope of this PR but as a quick fix I have added the table-responsive class to the table which basically adds a horizontal scroll on smaller screens. This area is in a table which will always be an issue on smaller screens. Maybe something like #13769 could be applied if the horizontal scroll is not sufficient (separate pr).

@brianteeman
Copy link
Contributor

We have a PR for the permissions already see #27670

@infograf768
Copy link
Member

infograf768 commented Jan 30, 2020

@ciar4n
Better, but not enough indeed. specially for the dropdowns.
may I suggest a change?

add

#page-filters .custom-select {
	width: auto;
}

in administrator/templates/atum/scss/blocks/_form.scss
towards the end of the file.
npm

Before patch

beforeconfig

After patch

afterconfig

@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 30, 2020

@infograf768 I'll leave it for a separate PR. As mentioned there is a PR for the permissions already. This PR is for the field control-group. The permissions is not even in a control-group.

@infograf768
Copy link
Member

This proposal above does not concerns Permissions, but the Text Filters.
#27670 concerns only Permissions... and works well for that.

@infograf768
Copy link
Member

I have tested both Permissions PR and this PR.
They are Ok for what they do.
Will make a new PR for Text Filters after they are merged.
In the mean while, I merge this one as it solves many issues.

@infograf768 infograf768 merged commit a9f80d6 into joomla:4.0-dev Jan 30, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 30, 2020
@infograf768
Copy link
Member

Tks.

@infograf768 infograf768 added this to the Joomla 4.0 milestone Jan 30, 2020
@ciar4n ciar4n deleted the control-group-flex branch January 30, 2020 10:50
@ciar4n
Copy link
Contributor Author

ciar4n commented Jan 30, 2020

Thank you for the tests!

brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
* Flex fields

* add form-vertical class

* switcher

* full width note

* reduce label padding

* Update switcher.scss

* increase vertical spacing between fields

* remove top margin from first field

* add table-responsive class to global filters

Co-authored-by: infograf768 <infografjms@gmail.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants