? ? NPM Resource Changed Pending

User tests: Successful: Unsuccessful:

avatar drmenzelit
drmenzelit
7 Apr 2021

Pull Request for Issue #33042 and #33037 .

Summary of Changes

Extended existent Javascript (shared-categories-accordion.js) to a collapse function for categories with subcategories.
Removed Bootstrap collapse attributes
Added a css for the view

Testing Instructions

See issues

Actual result BEFORE applying this Pull Request

See issues

Expected result AFTER applying this Pull Request

The collapse function works with and without published menu (see explanation in issue #33042

grafik

Documentation Changes Required

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
4.00

avatar drmenzelit drmenzelit - open - 7 Apr 2021
avatar drmenzelit drmenzelit - change - 7 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 7 Apr 2021
Category Repository NPM Change JavaScript Front End com_content
avatar drmenzelit drmenzelit - change - 7 Apr 2021
Labels Added: NPM Resource Changed ?
avatar chmst
chmst - comment - 7 Apr 2021

What do you think about removing line 29 in components/com_content/tmpl/categories/default_items.php? Or at least hide it?
We have nowhere else a text for article counts.

avatar drmenzelit
drmenzelit - comment - 7 Apr 2021

@Fedik it is possible to solve this with Joomla custom elements? I saw a collapse.js, but I'm not good in Javascript and I'm not sure if it can be used in replacement of Bootstrap collapse.

avatar Fedik
Fedik - comment - 7 Apr 2021

What you did is also okay.

If you mean bootstrap/collapse.js, it part of Bootstrap 5 changes
https://getbootstrap.com/docs/5.0/components/collapse/

avatar dgrammatiko
dgrammatiko - comment - 7 Apr 2021

it is possible to solve this with Joomla custom elements?

Yes and No. Yes as the collapse.js will do the accordion thing but No as the repo is basically abandoned. Even the existing elements that Joomla is using have several defects...

@Fedik she means https://github.com/joomla-projects/custom-elements/blob/master/src/js/collapse/collapse.js which in theory should be CSS framework agnostic

avatar drmenzelit
drmenzelit - comment - 8 Apr 2021

@Fedik , @dgrammatiko thank you guys for the answers

avatar hans2103
hans2103 - comment - 10 Apr 2021

@drmenzelit what if... we change it into <detail> and <summary>?
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

It will collapse out of the box.

avatar dgrammatiko
dgrammatiko - comment - 10 Apr 2021

what if... we change it into and

We are already doing it in

<details open>
<summary>${this.summarytext}</summary>
<div class="">
<div class="form-group">
<div class="input-group">
<label class="input-group-text" for="${this.parentId}-alt">${this.alttext}</label>
<input class="form-control" type="text" id="${this.parentId}-alt" />
</div>
</div>
<div class="form-group">
<div class="form-check">
<input class="form-check-input" type="checkbox" id="${this.parentId}-alt-check">
<label class="form-check-label" for="${this.parentId}-alt-check">${this.altchecktext}</label>
<div><small class="form-text text-muted">${this.altcheckdesctext}</small></div>
</div>
</div>
<div class="form-group">
<div class="form-check">
<input class="form-check-input" type="checkbox" id="${this.parentId}-lazy" checked>
<label class="form-check-label" for="${this.parentId}-lazy">${this.lazytext}</label>
</div>
</div>
<div class="form-group">
<div class="input-group">
<label class="input-group-text" for="${this.parentId}-classes">${this.classestext}</label>
<input class="form-control" type="text" id="${this.parentId}-classes" />
</div>
</div>
<div class="form-group">
<div class="input-group">
<label class="input-group-text" for="${this.parentId}-figclasses">${this.figclassestext}</label>
<input class="form-control" type="text" id="${this.parentId}-figclasses" />
</div>
</div>
<div class="form-group">
<div class="input-group">
<label class="input-group-text" for="${this.parentId}-figcaption">${this.figcaptiontext}</label>
<input class="form-control" type="text" id="${this.parentId}-figcaption" />
</div>
</div>
</div>
</details>`;

avatar drmenzelit
drmenzelit - comment - 10 Apr 2021

@drmenzelit what if... we change it into <detail> and <summary>?
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

It will collapse out of the box.

That is a good idea! I will look into it

avatar drmenzelit
drmenzelit - comment - 26 Jun 2021

summary / detail will not fit in this case, because we need a link to the category too.

avatar joomdonation
joomdonation - comment - 24 Jul 2021

I tested this and it worked. So if one of our frontend dev looks at this and confirm that the solution is OK, I will post my test result. Hope it will be merged before next RC.

avatar Fedik
Fedik - comment - 25 Jul 2021

It looks good to me,
I only think that the CSS added here need to be part of the template style

avatar drmenzelit
drmenzelit - comment - 25 Jul 2021

I think a little bit of CSS is important to make the categories list looks good independently of the template used (we have CSS also on some core modules). We can improve things in Cassiopeia and other templates can use or override the CSS here.

avatar dgrammatiko
dgrammatiko - comment - 25 Jul 2021

I only think that the CSS added here need to be part of the template style

No, the css should not be monolithic. Joomla is modular and the css/js should be modular

avatar joomdonation
joomdonation - comment - 25 Jul 2021

@dgrammatiko So this PR is fine as how it is? Or should the css/js code be moved to system assets so that we can use accordion anywhere we want? If it is fine, I will submit my test result as I tested the feature and it is working well.

avatar dgrammatiko
dgrammatiko - comment - 25 Jul 2021

Or should the css/js code be moved to system assets so that we can use accordion anywhere we want?

Good question. Turns out that this is the 3rd accordion that Joomla 4 will ship:

  • Bootstrap accordion
  • Joomla Custom Elements tabs (which also renders accordions)
  • this one

But then again if this is working as expected and passes the a11y reqs it's fine

avatar drmenzelit
drmenzelit - comment - 25 Jul 2021

it is possible to solve this with Joomla custom elements?

Yes and No. Yes as the collapse.js will do the accordion thing but No as the repo is basically abandoned. Even the existing elements that Joomla is using have several defects...

@Fedik she means https://github.com/joomla-projects/custom-elements/blob/master/src/js/collapse/collapse.js which in theory should be CSS framework agnostic

@dgrammatiko I asked for Joomla Custom Elements before...
I don't mind, if this PR get rejected, if there is another thing that do the job. I wanted to have a framework agnostic solution....

avatar dgrammatiko
dgrammatiko - comment - 25 Jul 2021

@dgrammatiko I asked for Joomla Custom Elements before...

I know and I answered that you shouldn't use that code (collapse) which is still true today. What changed is that I rewrote from scratch the Tabs but for this particular instance a plain (ie non CE) implementation is better (ie less code)
The PR is totally fine as is

avatar dgrammatiko
dgrammatiko - comment - 25 Jul 2021

@drmenzelit since the js file is loaded as a module the code shouldn't use the iife, should be like:

/**
 * @copyright  (C) 2018 Open Source Matters, Inc. <https://www.joomla.org>
 * @license    GNU General Public License version 2 or later; see LICENSE.txt
 */

if (!Joomla || !Joomla.Text) {
  throw new Error('core.js was not properly initialised');
}

// Selectors used by this script
const buttonsSelector = '[id^=category-btn-]';

/**
 * Handle the category toggle button click event
 * @param event
 */
const handleCategoryToggleButtonClick = ({ currentTarget }) => {
  const button = currentTarget;
  const icon = button.querySelector('span');

  // Toggle icon class
  icon.classList.toggle('icon-plus');
  icon.classList.toggle('icon-minus');

  // Toggle aria label, aria-expanded
  const ariaLabel = button.getAttribute('aria-label');
  const ariaExpanded = button.getAttribute('aria-expanded');
  button.setAttribute(
    'aria-label',
    (
      ariaLabel === Joomla.Text._('JGLOBAL_EXPAND_CATEGORIES') ? Joomla.Text._('JGLOBAL_COLLAPSE_CATEGORIES') : Joomla.Text._('JGLOBAL_EXPAND_CATEGORIES')
    ),
  );
  button.setAttribute(
    'aria-expanded',
    (
      ariaExpanded === 'false' ? 'true' : 'false'
    ),
  );

  const target = button.nextElementSibling;
  target.toggleAttribute('hidden');
};

Array.from(document.querySelectorAll(buttonsSelector)).forEach((button) => {
  button.addEventListener('click', handleCategoryToggleButtonClick);
});
avatar Fedik
Fedik - comment - 25 Jul 2021

No, the css should not be monolithic. Joomla is modular and the css/js should be modular

If you look in CSS it doing nothing "category specific", it just a general styling like display:flex and stuff, which I suspect can be done with .d-flex and other bootstrap. classes.

It not much important to me, I only saying that it can be done without this file

avatar drmenzelit
drmenzelit - comment - 26 Jul 2021

I have modified the JS file as suggested by @dgrammatiko

avatar dgrammatiko
dgrammatiko - comment - 26 Jul 2021

@drmenzelit you have to push the changes to Github...

avatar drmenzelit
drmenzelit - comment - 26 Jul 2021

@drmenzelit you have to push the changes to Github...

My notebook is driving me crazy today... push is done now

avatar joomdonation joomdonation - test_item - 29 Jul 2021 - Tested successfully
avatar joomdonation
joomdonation - comment - 29 Jul 2021

I have tested this item successfully on acca0c3

I don't have good enough frontend skill to confirm the solution is valid or not. But it is working as expected, tested it again today.


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

avatar hans2103 hans2103 - test_item - 30 Jul 2021 - Tested successfully
avatar hans2103
hans2103 - comment - 30 Jul 2021

I have tested this item successfully on acca0c3


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

avatar chmst chmst - change - 30 Jul 2021
Status Pending Ready to Commit
avatar chmst
chmst - comment - 30 Jul 2021

RTC


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

avatar wilsonge
wilsonge - comment - 30 Jul 2021

One question about presets otherwise fine to merge for 4.0.0 as this is core functionality. Thanks for reviewing the JS @dgrammatiko !

avatar wilsonge wilsonge - change - 9 Aug 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-08-09 10:48:21
Closed_By wilsonge
Labels Added: ? ?
Removed: ?
avatar wilsonge wilsonge - close - 9 Aug 2021
avatar wilsonge wilsonge - merge - 9 Aug 2021
avatar wilsonge
wilsonge - comment - 9 Aug 2021

Thanks!

Add a Comment

Login with GitHub to post a comment