? Success

User tests: Successful: Unsuccessful:

avatar nvyush
nvyush
23 Aug 2016

Pull Request for Issue # .

Summary of Changes

Add missing css class row to the file '/components/com_contact/views/category/tmpl/default_items.php' for unpublished contacts.

Testing Instructions

On admin panel:
1) Create some test contacts, unpublish a few of them.
2) Create test menu item of type 'Contacts' - 'List Contacts in a Category'.
3) Set the style 'protostar' as default.
On site:
1) Login as administrator.
2) Go to the test page.
image

Documentation Changes Required

None

avatar joomla-cms-bot joomla-cms-bot - change - 23 Aug 2016
Category Front End Components
avatar nvyush nvyush - open - 23 Aug 2016
avatar nvyush nvyush - change - 23 Aug 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Aug 2016
Labels Added: ?
avatar andrepereiradasilva andrepereiradasilva - test_item - 23 Aug 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 23 Aug 2016

I have tested this item successfully on fe29aed

reproduced issue.
patch works as described


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

avatar C-Lodder
C-Lodder - comment - 23 Aug 2016

This fixes the issue but it pushes the background of the row outside the parent container. Not sure who did the markup for this initially but they need to understand the use of a row.

The row class should be removed from all <li> elements and properly fixed

avatar zero-24
zero-24 - comment - 23 Aug 2016

@c-lodder do you have another idea how to fix that?

avatar andrepereiradasilva
andrepereiradasilva - comment - 23 Aug 2016

i think it was like that before.

avatar C-Lodder
C-Lodder - comment - 23 Aug 2016

Either replace all the row classes on <li> elements with clearfix....OR...because the grid systsem needs to be applied, don't use a list

avatar bhavikTailored
bhavikTailored - comment - 23 Aug 2016
  • I have successfully tested this issue.
  • I have attached Screen Shot.

screen shot 2016-08-23 at 05 57 39


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

avatar bhavikTailored bhavikTailored - test_item - 23 Aug 2016 - Tested successfully
avatar bhavikTailored
bhavikTailored - comment - 23 Aug 2016

I have tested this item successfully on 40305ea


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

avatar C-Lodder C-Lodder - test_item - 23 Aug 2016 - Tested successfully
avatar C-Lodder
C-Lodder - comment - 23 Aug 2016

I have tested this item successfully on 40305ea

Thanks for updating the markup ;)


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

avatar zero-24 zero-24 - change - 23 Aug 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 23 Aug 2016

RTC. Thanks!


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

avatar joomla-cms-bot joomla-cms-bot - change - 23 Aug 2016
Labels Added: ?
avatar rdeutz rdeutz - change - 25 Aug 2016
Status Ready to Commit Needs Review
Labels
avatar rdeutz
rdeutz - comment - 25 Aug 2016

I don't think we can change the markup and replace an ul with a table


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

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2016
Labels Removed: ?
avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

@rdeutz - why?

avatar rdeutz
rdeutz - comment - 25 Aug 2016

@C-Lodder It might break templates and sites

avatar rdeutz
rdeutz - comment - 25 Aug 2016

set to needs review, because I have too look what is our policy for template changes. If someone knows it out of his head please set the milestone


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

avatar brianteeman
brianteeman - comment - 25 Aug 2016

6.1.7 Rendered markup

For the time being, rendered markup is not subject to our backwards
compatibility promise. We will try not to change markup in such a way that
a site might render differently, but we can't promise not to break anything
at the present time. We will work on defining ways in which we might make a
backwards compatibility promise for markup in the future, but we do not
currently have a satisfactory consensus on a workable standard.

avatar rdeutz
rdeutz - comment - 25 Aug 2016

Thanks @brianteeman

avatar brianteeman
brianteeman - comment - 25 Aug 2016

Its a tricky one as we quite often accept PR that will change the rendered markup - although usually in very subtle ways. I tend to think changing a ul to a table is not a subtle way and "could" have a bigger impact. That being said I am edging more in favour of this PR as its a clear bug and it is a relatively less used page. (I wouldnt be in favour of a change to com_content lists)

avatar nvyush
nvyush - comment - 26 Aug 2016

@brianteeman If replace list-striped with row-striped for ul and replace row with row-fluid for li we can keep the old markup, but this solution has a little bug marked of the red arrow. If this solution is acceptable I can commit it to PR.

image

avatar C-Lodder
C-Lodder - comment - 26 Aug 2016

That can also work. The width is not correct because someone tried to be clever and use:

.row-striped .row-fluid {
    width: 97%;
}

when it should be:

.row-striped .row-fluid {
    width: 100%;
    box-sizing: border-box;
}

If you want to submit a PR with your changes and the CSS change I've given you above, I'll be in favour of it.

avatar nvyush
nvyush - comment - 26 Aug 2016

@C-Lodder I don't know who and why set width: 97%;. I do not know to what side effects will lead this solution.
UPD: May be move the changing of CSS to a new PR?

avatar C-Lodder
C-Lodder - comment - 26 Aug 2016

@nvyush - The row-striped class is only being used on 3 HTML elements in the entire CMS, as shown in the screenshot below:
screeny

I've tested the CSS update from my previous comment and it works perfectly fine.

avatar C-Lodder
C-Lodder - comment - 26 Aug 2016

PR submitted: #11807

avatar nvyush nvyush - change - 26 Aug 2016
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2016-08-26 12:18:24
Closed_By nvyush
avatar nvyush nvyush - close - 26 Aug 2016
avatar nvyush
nvyush - comment - 26 Aug 2016

Closed as we have a PR for testing #11807

Add a Comment

Login with GitHub to post a comment