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] Normalise Featured Contact List #30868

Merged
merged 6 commits into from
Oct 7, 2020

Conversation

infograf768
Copy link
Member

Summary of Changes

Follow up on #30813 and #30859
Warning: these 2 PR have to be present in your test site to test the Clear button.

npm ci has to be applied

Changes:
Normalises the table display
Adds a Title Search Filter, Filter and Clear buttons.
a11y legend and scope added
Modified the search placeholder (it is also used in Contact List but I forgot to modify it there)
Used Title instead of Name for column heading, as in Contact list

Testing Instructions

Create some contacts and set them to featured.
Create a Featured Contacts menu item.
Display in frontend.

After patch, test that the Filter and the Clear button work as should

Actual result BEFORE applying this Pull Request

Screen Shot 2020-10-02 at 11 50 02

Expected result AFTER applying this Pull Request

Screen Shot 2020-10-02 at 12 14 04

NOTE

No idea why we display the NUM in the first column.
If nobody knows why, we can get rid of it.

Meaanwhile, I kept in the code

					<tr class="<?php echo ($i % 2) ? 'odd' : 'even'; ?>" itemscope itemtype="https://schema.org/Person">
						<td class="item-num">
							<?php echo $i; ?>
						</td>

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Oct 2, 2020
@chmst
Copy link
Contributor

chmst commented Oct 2, 2020

I'd like to get rid of the Number!

@infograf768
Copy link
Member Author

I'd like to get rid of the Number!

Me too, if it is BC.

@brianteeman
Copy link
Contributor

There is no b/c issue

@chmst
Copy link
Contributor

chmst commented Oct 2, 2020

Tested with success. If you remove the number column, I'll re-test.


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

@infograf768
Copy link
Member Author

There is no b/c issue

@brianteeman
Do you mean I can get rid of that column?

Any idea about this very specific <tr>
<tr class="<?php echo ($i % 2) ? 'odd' : 'even'; ?>" itemscope itemtype="https://schema.org/Person">

Should I replace it with the same stuff we have in the category list?

						<?php if ($this->items[$i]->published == 0) : ?>
							<tr class="system-unpublished featured-list-row<?php echo $i % 2; ?>">
						<?php else : ?>
							<tr class="featured-list-row<?php echo $i % 2; ?>" >
						<?php endif; ?>

@brianteeman
Copy link
Contributor

  1. yes - its just a display you are not removing data

  2. that css is/was to create "stripes"

@infograf768
Copy link
Member Author

@chmst @brianteeman
please test again
also moved Unpublished badge below contact title (log to see)
Screen Shot 2020-10-03 at 09 19 16

<form action="<?php echo htmlspecialchars(Uri::getInstance()->toString()); ?>" method="post" name="adminForm" id="adminForm">
<?php if ($this->params->get('filter_field') || $this->params->get('show_pagination_limit')) : ?>
<fieldset class="com-contact-featured__filters filters">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filter and list limit are not a fieldset. They are not related to each other.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at similar code for
components/com_content/tmpl/category/default_articles.php

<input type="hidden" name="filter_order" value="<?php echo $listOrder; ?>">
<input type="hidden" name="filter_order_Dir" value="<?php echo $listDirn; ?>">
</fieldset>
<legend class="sr-only"><?php echo Text::_('COM_CONTACT_FORM_FILTER_LEGEND'); ?></legend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for a legend

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fieldset needs a legend (and wave asks for it): see

<fieldset class="com-content-category__filters filters btn-toolbar clearfix">
<legend class="sr-only"><?php echo Text::_('COM_CONTENT_FORM_FILTER_LEGEND'); ?></legend>

<fieldset class="com-contact-category__filters filters btn-toolbar">
<legend class="sr-only"><?php echo Text::_('COM_CONTACT_FORM_FILTER_LEGEND'); ?></legend>

@chmst
Copy link
Contributor

chmst commented Oct 3, 2020

The list is a big improvement.

For the filter, I have some doubts.

  • The form is only the filter, not the whole list
  • Search field and list limit are no fieldset, they don't belong together
  • The clear button does not clear the input.

@infograf768
Copy link
Member Author

infograf768 commented Oct 3, 2020

The clear button works if

Follow up on #30813 and #30859
Warning: these 2 PR have to be present in your test site to test the Clear button.

npm ci has to be applied

@infograf768
Copy link
Member Author

@chmst
I replied to your concerns above, I guess.

@brianteeman
Copy link
Contributor

brianteeman commented Oct 3, 2020 via email

@infograf768
Copy link
Member Author

infograf768 commented Oct 3, 2020

The point was that it's not a fiedset

Then you may have to change the code for articles in 3.x and for articles and contacts lists in 4.0-dev, which you tested OK

https://github.com/joomla/joomla-cms/blob/staging/components/com_content/views/category/tmpl/default_articles.php#L67-L104

@brianteeman
Copy link
Contributor

I have no opinion on if it should be a fieldset or not. I was merely explaining why @chmst said that you should not have the legend.

@infograf768
Copy link
Member Author

Animated gif concerning the Clear filter.
limitbox_contacts-list-animated

@infograf768
Copy link
Member Author

I obviously can take off the part concerning fieldset, here and in List Contacts in a Category, if desired.

@infograf768
Copy link
Member Author

I am asking @zwiastunsw to please help on this decision as we have only one input (the searchfield)
One aspect is sure: if we keep the fieldset, it must have a legend.

@infograf768
Copy link
Member Author

After discussing with @chmst and @zwiastunsw fieldset is not necessary.
Reworked the tmpl.

Please test.

Note

Another PR will follow for contacts list and articles list.

@chmst
Copy link
Contributor

chmst commented Oct 6, 2020

I have tested this item ✅ successfully on 510dc5f


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

1 similar comment
@richard67
Copy link
Member

I have tested this item ✅ successfully on 510dc5f


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 6, 2020
@infograf768
Copy link
Member Author

title removed as requested.
I guess no need to test again.

@HLeithner HLeithner merged commit 1c7442c into joomla:4.0-dev Oct 7, 2020
@HLeithner
Copy link
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 7, 2020
@HLeithner HLeithner added this to the Joomla 4.0 milestone Oct 7, 2020
@infograf768 infograf768 deleted the 4.0_featured_contacts_list branch October 7, 2020 08:16
@infograf768
Copy link
Member Author

Similar PR for articles
#30981

sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* taking off NUM column

* adding back itemscope

* string used in form

* Getting rid of fieldset

* removing title as required
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants