[#10011] - Update Joomla! content and contacts to include an item-featured class if an item is featured
- Closed
- 29 Jun 2019
- Medium
- Build: staging
- # 10011
- Diff
- uglyeoin:staging
- Success hound No violations found. Woof! Details
- Failure continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request Details
- Success continuous-integration/drone/pr the build was successful Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #10011 .
Summary of Changes
The class item-featured has been added to both components and modules that contain a featured item. Featured items include articles and contacts.
Testing Instructions
Install Joomla! & Patch Tester
Install Joomla! & Patch tester. Here's a video with some instructions. https://youtu.be/4OWgusZgIfk
Download here: https://docs.joomla.org/Component_Patchtester_for_Testers
The pull ID for this issue is 10011
The title is Update Joomla! content and contacts to include an item-featured class if an item is featured.
Test the issues
You will need to create (at least) 2 articles (if using sample data you can use these items). One should be a featured article, one should not be a featured article. You will need to ensure that you show both of these in your test. The easiest way to do this in a blog view is to ensure they are in the same category and the category is selected. The same is true for other views and modules.
You will need to create (at least) 2 contacts. One should be a featured contact one should not be a featured contact. You will need to ensure both of these are showing on the page you are viewing.
You will need to create menu items for the category views listed below. So for example you will need to create a menu for category blog view, article archived view, contact all contacts view.. etc. If any views are not mentioned but include an article or a contact, you should add these to your testing process and report back if featured items do not show the class item-featured.
The below list outlines the modules and components that have been changed.
Components
You should create a menu item for each of the following.
com_content - category blog view
com_content - category list view
com_content - archive view
com_content - featured articles view
com_contact - category view
com_contact - featured view
Modules
You should create and publish all of these modules. They are all part of the Joomla! core. Simply go to module manager, click on new and find the correct module. Publish it to the page you are looking at (or all pages).
mod_article_archive
mod_articles_category
mod_articles_latest
mod_articles_news - horizontal view
mod_articles_news - vertical view
mod_articles_most_popular
mod_related_items
Once you have done this, you should navigate to the menu item you have created, for example you should have created a menu item that links to category blog and contains the articles you created. Visiting the blog page should show you your articles, you should have selected one to be a featured article.
You will not notice any immediate difference, you will have to inspect the code to see if the class "item-featured" shows. You will need to repeat this step for all of the different content views.
The same is true for the modules, again, you will need to ensure they are showing on your page, and inspect the element to see if the class item-featured shows.
Finally, you will need to repeat these steps for the contact pages. The contacts that are featured should be called item-featured.
The featured contacts/content should all have item-featured as a class. Any items that you have NOT set to be featured, should not include this class.
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Category | ⇒ | Components Feature Request Templates (site) |
Personally I am not in favour of this as if you already have some styling in the template for the class featured your content display will be changed when you update joomla without your knowledge or control. To me this is the role of the template and a template override.
But if this is going to be accepted then you will need to add this to the beez template as well.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
Would changing the class name to something like art-featured be a solution? The problem being without solving the issue in the core, you're really only solving it for people who use the default template.
Not using an existing classname would solve the first part of my comment
On 20 April 2016 at 19:03, uglyeoin notifications@github.com wrote:
Would changing the class name to something like j-art-featured be a
solution?—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#10011 (comment)
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
Yes you should have done it as one (and at least given correct test
instructions in the other one)
I have explained how you should add those changes to this pull request there
On 20 April 2016 at 19:12, uglyeoin notifications@github.com wrote:
I have updated Beez as per the later part of your comment. See here:
#10014 #10014 My apologies if
I should have done it all as one. I've used the class .art-featured on both.—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#10011 (comment)
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
For items in featured it isn't needed and I have my doubts that this is needed at all.
Ah ha I achieved the goal of merging them. Thanks for your help @brianteeman & @rdeutz .
@rdeutz can you explain your comment as why it isn't needed? My aim is to be able to style featured articles differently to normal ones. Is that already possible somehow?
@uglyeoin for leading items you could use .items-leading div {} and you can do the same, to style an item after the leading items you need your change but as I said I have my doubts that this needed at all. I never had such a requirement and you can do this easily with an overwrite for your template. At this state I don't think that is something we should put into the core.
I think if you have a feature called Featured in the core, you should be able to accurately select it for CSS styling without having to override the system. Sounds like a half baked feature to me.
The object of the Featured feature is to be able to select articles from
multiple categories and display them on a single page with the featured
articles menu type. It is not to be able to format an article wherever it
appears. This is perhaps one of the oldest features of joomla that has been
in existence for over ten years. As stated before if you want to apply
special formatting to a featured article in a regular category blog view
then you can do this with a template override.
On 21 April 2016 at 12:03, Gary Barclay notifications@github.com wrote:
I think if you have a feature called Featured in the core, you should be
able to accurately select without having to override the system. Sounds
like a half backed feature to me.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
You can do an override, yes. But what is the point in a featured article if it has no visual difference from any other article. The very definition of featured is "have as a prominent attribute or aspect".
A featured article may be a leading item or it may not be a leading item. Styling all of the leading items with .leading-item div is not selecting featured items.
In the featured menu item it is easy to select all articles. In a mixed situation like blog. There is no way to achieve this at present.
Square Balloon
On 21 Apr 2016, at 12:07, Brian Teeman > wrote:
The object of the Featured feature is to be able to select articles from
multiple categories and display them on a single page with the featured
articles menu type. It is not to be able to format an article wherever it
appears. This is perhaps one of the oldest features of joomla that has been
in existence for over ten years. As stated before if you want to apply
special formatting to a featured article in a regular category blog view
then you can do this with a template override.
On 21 April 2016 at 12:03, Gary Barclay > wrote:
I think if you have a feature called Featured in the core, you should be
able to accurately select without having to override the system. Sounds
like a half backed feature to me.You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub#10011 (comment)
I have already explained the purpose of the featured article. Not repeating
myself again.
The great thing abut joomla is that if you dont like a core behaviour then
you can override it
On 21 April 2016 at 12:20, uglyeoin notifications@github.com wrote:
You can do an override, yes. But what is the point in a featured article
if it has no visual difference from any other article. The very definition
of featured is "have as a prominent attribute or aspect".A featured article may be a leading item or it may not be a leading item.
Styling all of the leading items with .leading-item div is not selecting
featured items.In the featured menu item it is easy to select all articles. In a mixed
situation like blog. There is no way to achieve this at present.Square Balloon
On 21 Apr 2016, at 12:07, Brian Teeman notifications@github.com>> wrote:
The object of the Featured feature is to be able to select articles from
multiple categories and display them on a single page with the featured
articles menu type. It is not to be able to format an article wherever it
appears. This is perhaps one of the oldest features of joomla that has been
in existence for over ten years. As stated before if you want to apply
special formatting to a featured article in a regular category blog view
then you can do this with a template override.On 21 April 2016 at 12:03, Gary Barclay notifications@github.com>> wrote:
I think if you have a feature called Featured in the core, you should be
able to accurately select without having to override the system. Sounds
like a half backed feature to me.You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<
https://github.com/joomla/joomla-cms/pull/10011#issuecomment-212860589>—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
That's certainly not what Featured means to me, maybe we should rename it to Cross Category Filter?
After ten plus years the answer is no
On 21 Apr 2016 12:35 pm, "Gary Barclay" notifications@github.com wrote:
That's certainly not what Featured means to me, maybe we should rename it
to Cross Category Filter?—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)
"To feature" can - as far as I know as a not native English speaker but with good knowledge on that language - mean to present something at a prominent place, and that's what it does, so I am very OK with the naming as it is.
I have to say I mixed leading and featured a bit. I agree with Brian, the purpose of this is that you can show articles in a special view. I would say when we give articles a special class when they are featured then this should be true for all views articles are presented and not only for a blog view.
Again never had the need for it and you can do it with an overwrite, nothing for core
You could argue that, and I would say that makes sense. But in other views i.e. featured menu item you do not see featured/non-featured articles side by side, so it is less of an issue and could be styled via the menu.
I'm all for overrides, they are a great feature, but when an addition to the core is about 40 characters, and an override creates 4 new files, I think an override is overkill. I was surprised that there was no way to style a featured article.
I had a requirement for this feature, so I used an override. It just struck me as strange that featured articles could not be featured, unless you sorted them by featured first. Surely the fact that ordering them first is an option shows that others have thought about this issue and want a way to feature featured articles?
Featured articles are rendered null and void in nearly all of these ordering options. I wonder if there are any other designers out there that would agree with me?
Regardless as to whether you have found a need for this feature, others have. Myself, and these guys: http://forum.joomla.org/viewtopic.php?f=624&t=773238
I can actually see this being very useful. If you "feature" an article in your site, having a class around that article ANYWHERE it appears means you could do things like change the link color, or add a little star next to it. It would take a lot of work to carry that through the entire system, but I think it WOULD be useful. As far as the "purpose" of the Featured articles function, anyone new to Joomla isn't going to know that there's a 10 year old purpose for that function, they're going to wonder why they can't style a featured article somewhere else without making overrides everywhere.
Basically we can 2 ways of thinking:
No, we won't think outside the box, we won't expand that feature and it will stay useful in only one view. Stop asking.
Yeah, that's a good idea, it would expand the use of Featured articles into other views. This can only be a good thing!
I have to be honest, I'm really tired of seeing excuses like #1 crop up again and again when someone suggests a new feature.
@richard67 the only time it places them in a prominent position if if you sort by featured first. Any other sorting does not give them any more prominence than a standard article.
@uglyeoin No, it places them in a prominent position in the way as @brianteeman tried to explain you, e.g. on the front page.
@richard67 Only if you have the menu item set up like that on the front page. So they are only ever featured if you either set up the menu item Featured articles, or you sort by featured articles on the blog view. Anywhere else they have no prominence, that is what this pull request solves.
I guess I just will never understand why we argue over a tiny code change that can benefit people and make a useful feature more useful.
I actually think this is a useful addition. It will make it possible to style a featured article differently when that article appears in a regular list, without requiring an override. Less work for designers. That's good.
It probably needs a bit more work to be applied to other sections of the code, like the featured articles module. But I'm not going to go to any effort if it's just going to be shouted down.
Personally I also think it's a nice and simple addition where I can see a usecase for people.
Also he implementation is done fine and there will be nothing backfiring here.
from me.
+1
I have tested this item 
successfully on 19a9927
I too see this as useful.
I had preferred the class name to be just featured, but anyway...
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
What about featured contacts as well
@smz The problem was that "featured" was already used elsewhere in some css, so it could have caused problems.
I think what @uglyeoin has chosen at the end, "featured-article", is ok and telling, in opposite to "art-featured" as it was a few commits before.
For the contacts it should be "featured-contact" then.
To which other (article) views should this applied? Not to "Featured articles", I guess, where it would be redundant... Would it be useful on "Category list"?
I have tested this item successfully on 19a9927
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
@smz No, in category lists articles do not appear.
I think it could be added to the featured view for consistency, but there are pros and cons for that.
The contacts should be done, too, but this can be done in a new PR, too.
So I think I can test this here with success, and if @uglyeoin will add the featured and contacst to this PR here, then we can test it again, no problem for me.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
| Labels |
Added:
?
|
||
Sure will, I may need someone to talk me through adding it all to this merge but I will try first and ask if i get stuck.
These modules would also need it too right? If any are wrong please say (giving a reason) and if any are missing please say.
Latest articles module
Articles Newsflash
Articles Related
Articles Cateogory
Plus contacts as they can be featured as well.
On 22 Apr 2016 12:32 pm, "uglyeoin" notifications@github.com wrote:
These modules would also need it too right? If any are wrong please say
(giving a reason) and if any are missing please say.
Latest articles module
Articles Newsflash
Articles Related
Articles Cateogory—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10011 (comment)
Probably all the tags views will need updating as well
There are no contact modules though right?
Can you have featured tags? Or do you mean on those pages there could be featured articles?
Can you have featured tags? Or do you mean on those pages there could be featured articles?
He means on views for tagged articles there may be featured articles and not featured articles.
Does this PR also add a featured class to the article itself? Would be pretty handy. This would allow me to indicate that the article has been featured by using something like:
.item-page.featured .page-header:before {
content: "Featured";
background: #eee;
}
While a dedicated menu item showing featured articles and having a class assigned to it could technically work, the majority of us will want categorized content that shows both featured and non featured articles. As a result we cannot apply the featured class to the menu item itself.
Yes it adds that class, except I have used the classes .featured-article, .featured-tag, .featured-contact.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
Ok chaps, it's fairly late, my apologies if there are any mistakes. I'm not sure if the contacts one will work so please test thoroughly (test everything thoroughly). I'm not a coder, so I welcome any feedback, suggestions, or other teachings.
... BTW I searched for the simple "featured" class in our CSS and couldn't find it. Are you sure it is already used in other contexts?
... BTW I searched for the simple "featured" class in our CSS and couldn't find it. Are you sure it is already used in other contexts?
@smz even if it is not, there's a chance it's a common enough phrase that it could be used in other templates or modules. I think we should err on the side of safety.
For the tags I used the class .featured-tag. Looking back this morning with fresher eyes, should these remain as .featured-article??
If it is the articles and not the tags which are featured (what is the case as far as I understand): Sure.
If it is the articles and not the tags which are featured (what is the case as far as I understand): Sure.
I agree, but (not being familiar with our tags implementation and having given just a cursory look at the code) I have a doubt: are tags just for com_content or is it an extensible mechanism that can be applied to whatever component (same as categories)?
Also, your "tags code" seems to be out of sync with current "staging" that has received a commit 14 days ago (07f29eb). I'm not sure this is the only conflict preventing the merge of this PR...
... BTW I searched for the simple "featured" class in our CSS and couldn't find it. Are you sure it is already used in other contexts?
@smz even if it is not, there's a chance it's a common enough phrase that it could be used in other templates or modules. I think we should err on the side of safety.
I guess this is a new form of B/C... "Preventive Backward Compatibility".
Jokes apart, I think that statistically the odds to collide with a previously unused (by the core) CSS class potentially used by users (and that would very probably be in overrides...) are about the same, unless... we decide (and this is something for the PLT) to prefix our core classes with something specific (maybe just a "J") in order to minimize collision probability and we publicly declare that this is the case. This of course can't be done for old classes but it could be done for newly added CSS classes, from now on.
featured-article is perfectly fine. Maybe if you want consistency with the existing class for the blog layout (blog-featured) you could turn it around to article-featured but it doesn't really matter to me.
The featured class may also be present on templates which include the view name as class, but ideally they do it like Protostar where you get a view-featured class.
I wouldn't add a prefix to CSS just to mimic a "namespace".
@bakual What about the "tags" thing? You very likely know better than I do...
I haven't looked at it, but since the tags don't have a featured attribute I guess it would be about the tagged items. But I'm not sure how you would detect if it's an article, contact or a featured item from a 3rd party extension. So featured-article is probably wrong there, better use featured-item maybe?
And of course also test with tagged items that don't have a featured attribute. Not that it throws notices because the property is undefined 
... better use featured-item maybe?
Agreed, and IMHO it could be the same everywhere...
Thank you all for your comments. @Bakual I agree article-featured is better as it is more consistent.
@smz the other pull request for Tags looks like an improvement. What should I do, update my code to replicate it, or do you know if there a way to merge the two or something like that? Or should I just be pulling from a different source in the first place? Is there a way to tell prior to editing it?
item-featured vs article-featured. I throw it to the room, I don't care what it's called so long as the functionality works, but consistency is always helpful. Ideally you would prefer if you could predict what a class would be called if it existed. Joomla! could probably do with some written down rules on naming conventions (if they exist please aim me at them).
TBH I find both "item-featured" and "article-featured" ugly: in my syntax first comes the adjective, then the noun. But it is also really unimportant for me.
My vote goes to "featured-item" everywhere, anyway.
"featured-item" makes sense
@smz thanks. I think that would work. Everyone has their own way of working, and I'm not one to quarrel over which is best, the only thing I would like to achieve in this instance is to do things the "Joomla!" way to aid consistency.
I'm also aware that I have a typo on the tags where I have called it featured-contact, but I am awaiting feedback before changing things. Let's see what the others say before I go around making commits.
There was also a query as to whether the featured class is required on lists with links in them. If anyone has an opinion, let's hear it.
@uglyeoin beware of what @Bakual said here:
And of course also test with tagged items that don't have a featured attribute. Not that it throws notices because the property is undefined
This is really important and correct. What it means is that for tags (that can reference "items" of unknown nature) the "featured" property can be absent (as opposed to "set to null") and your code will issue a notice in those cases.
I think the correct way to test there should be (isset($item->featured)) instead of simply ($item->featured)
Edit: or (property_exists($item->featured) && $item->featured)
Edit: I'm also wondering if it is necessary at all to have this in com_tags...
I think the correct way to test there should be
(isset($item->featured))instead of simply($item->featured)
You'd need both, so if (isset($item->featured) && $item->featured). Just because it's present also doesn't necessarily mean that it's a featured item.
Depends on use cases I guess... http://php.net/manual/en/function.property-exists.php
Note:
As opposed with isset(), property_exists() returns TRUE even if the property has the value NULL.
EDIT: property_exists() would work fine on any class object (including PHP's stdClass), but NOT on arrays.
As opposed with isset(), property_exists() returns TRUE even if the property has the value NULL.
Yes, and hence the necessity of the second test: && $item->featured
But I'm wondering if isset() can't be view as a shorthand for the twos as our "fetaured" property is either null or "something"
just curious, what is wrong with good old if(!empty($foo->bar))? 
But I'm wondering if isset() can't be view as a shorthand for the twos as our "fetaured" property is either null or "something"
If the only values it can possibly be today, and ever possibly will be, equates to either null or true then the isset() check is good enough. If it can have multiple values (null, 0, 1), then you should do both isset AND check the value.
just curious, what is wrong with good old if(!empty($foo->bar))?
Nothing, as long as the possible values will always be something like null/true or null/0/1. If for whatever reason they started being null/Yes/No then you'd be in trouble.
just curious, what is wrong with good old if(!empty($foo->bar))?
The "bar" in it could make people get thirsty
TBH I think my property_exists($item->featured) && $item->featured is the more explicit...
See that's the thing about PHP. There's no One Right Way™. You may prefer property_exists($item->featured) && $item->featured, Fedik may prefer if(!empty($foo->bar)), but neither are technically wrong as long as both accomplish the job and will always only work with null/true/false values basically.
<ot>I've seen somewhere around, but can't remember where, a way to look at the generated opcodes/tokens generated by PHP code. Does anybody knows how to do that?</ot>
And that's why there are about a bajillion and seven code styles and opinions about how PHP code is structured
As for the OT question, 3v4l.org is a resource that lets you test small PHP code snippets (usually with "plain" PHP) and shows all the steps that PHP goes through processing it.
BTW, property_exists($item->featured) is blatantly wrong: substitute with property_exists($item, 'featured')
Sorry!
For the curious: @mbabker wins!
echo (property_exists($item, 'featured') && $item->featured) ? 'yes!' : 'no!';
line #* E I O op fetch ext return operands
-------------------------------------------------------------------------------------
2 0 E > CAST 8 ~1
1 ASSIGN !0, ~1
4 2 INIT_FCALL 'property_exists'
3 SEND_VAR !0
4 SEND_VAL 'featured'
5 DO_ICALL $3
6 > JMPZ_EX ~4 $3, ->9
7 > FETCH_OBJ_R $5 !0, 'featured'
8 BOOL ~4 $5
9 > > JMPZ ~4, ->12
10 > QM_ASSIGN ~6 'yes%21'
11 > JMP ->13
12 > QM_ASSIGN ~6 'no%21'
13 > ECHO ~6
14 > RETURN 1
echo (isset($item->featured) && $item->featured) ? 'yes!' : 'no!';
line #* E I O op fetch ext return operands
-------------------------------------------------------------------------------------
2 0 E > CAST 8 ~1
1 ASSIGN !0, ~1
4 2 ISSET_ISEMPTY_PROP_OBJ ~3 !0, 'featured'
3 > JMPZ_EX ~3 ~3, ->6
4 > FETCH_OBJ_R $4 !0, 'featured'
5 BOOL ~3 $4
6 > > JMPZ ~3, ->9
7 > QM_ASSIGN ~5 'yes%21'
8 > JMP ->10
9 > QM_ASSIGN ~5 'no%21'
10 > ECHO ~5
11 > RETURN 1
echo (!empty($item->featured) && $item->featured) ? 'yes!' : 'no!';
line #* E I O op fetch ext return operands
-------------------------------------------------------------------------------------
2 0 E > CAST 8 ~1
1 ASSIGN !0, ~1
4 2 ISSET_ISEMPTY_PROP_OBJ ~3 !0, 'featured'
3 BOOL_NOT ~4 ~3
4 > JMPZ_EX ~4 ~4, ->7
5 > FETCH_OBJ_R $5 !0, 'featured'
6 BOOL ~4 $5
7 > > JMPZ ~4, ->10
8 > QM_ASSIGN ~6 'yes%21'
9 > JMP ->11
10 > QM_ASSIGN ~6 'no%21'
11 > ECHO ~6
12 > RETURN 1
@uglyeoin You could also use
if (isset($item->featured) ? $item->featured : false)
I.e. use the ternary operator.
I don't know how it is in PHP, but often in languages which have such tinary operator, the tinary operator performs slightly better than any "if then else" or " && " because just clever shifting of variable in memory or a register, at least it is like this in C.
And if you use it more than 1 time in the same function, assign the isset($item->featured) ? $item->featured : false to a local variable and use that variable in the "if"s.
Blimey, I'm just a simple front end designer guys. I'll give it my best shot but would appreciate the enlightened amongst you having a look once I've made the changes. I'm going to run with item-featured. I'll try to do the changes a little later on this evening.
@richard67 @uglyeoin The ternary operator exist in PHP. The way it is used is as Richard has explained.
So, to be clear, I'm looking at using
(isset($item->featured) && $item->featured) ? ' item-featured' : '';
Right?
It does, thanks. And everyone's input has been very informative and helpful. I particularly liked the test to find the most efficient route, very informative. No doubt I will still get something wrong, but let's cross our fingers and one day we may well get a feature.
You couls also use ternary operator both times:
(isset($item->featured) ? $item->featured : false) ? ' item-featured' : '';
But maybe
(isset($item->featured) && $item->featured) ? ' item-featured' : '';
is better, don't know. The latter seems to be more readable.
Not sure if that was really your test case, but you had the value check duplicated. You only need echo (!empty($item->featured) ? 'yes!' : 'no!';
Correct! As empty() will return true also if the value is false or 0.
Then, for echo (!empty($item->featured) ? 'yes!' : 'no!'; we have:
line #* E I O op fetch ext return operands
-------------------------------------------------------------------------------------
2 0 E > CAST 8 ~1
1 ASSIGN !0, ~1
4 2 ISSET_ISEMPTY_PROP_OBJ ~3 !0, 'featured'
3 BOOL_NOT ~4 ~3
4 > JMPZ ~4, ->7
5 > QM_ASSIGN ~5 'yes%21'
6 > JMP ->8
7 > QM_ASSIGN ~5 'no%21'
8 > ECHO ~5
9 > RETURN 1
@Fedik wins and he'll offer everybody a drink at the "bar"!
to everyone
... and as a further micro-optimzation:
echo (empty($item->featured)) ? 'no!' : 'yes!';
`line #* E I O op fetch ext return operands
-------------------------------------------------------------------------------------
2 0 E > CAST 8 ~1
1 ASSIGN !0, ~1
4 2 ISSET_ISEMPTY_PROP_OBJ ~3 !0, 'featured'
3 > JMPZ ~3, ->6
4 > QM_ASSIGN ~4 'no%21'
5 > JMP ->7
6 > QM_ASSIGN ~4 'yes%21'
7 > ECHO ~4
8 > RETURN 1
But haven't we now removed @mbabker 's original point here:
just curious, what is wrong with good old if(!empty($foo->bar))?
Nothing, as long as the possible values will always be something like null/true or null/0/1. If for whatever reason they started being null/Yes/No then you'd be in trouble.
Or are we covered because we absolutely know the values and they will never change?
They shouldn't change. But if they ever do, the check would have to change because right now it basically only accepts boolean or null values (with PHP's weak typing this includes integer 0/1 and string '0' and '1'). If the field ever expanded to have a possible value of 2 then this check may not be sufficient anymore. It's just something to keep in mind if you add possible values to an existing field.
@mbabker I'm thinking that unless we can think of a possible reason there would be a third value we don't need to allow for every potential possibility in the future. (a) you are far more likely to understand if this is a sensible approach, and (b) can anyone think of an additional value that could possibly be used one day?
Right, keep it simple. empty($item->featured) is fine for the existing use cases today. If someone added a new case tomorrow, they'd need to do the relevant leg work to make sure the code works correctly with that new case.
Is this ready for testing yet or is it still a work in progress?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
| Milestone |
Removed: |
||
Sorry, I haven't done the amendments yet. Please don't test just yet as it will be wasting your time. If anyone else has a spare moment and wants to jump in and make the changes (is that possible?) I am not precious about it, I'm just a bit busy at the moment.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
I have made the changes, I do not understand how to resolve the conflicts, if someone can point me in the right direction as to see the conflicts so I can try to resolve them then we can test.
@uglyeoin I have a PR ready for you but your staging branch is very outdated. First you will need to update the staging branch before I can send it to you. You will need to do the following if you use git on the command line:
git pull upstream staging
git push origin
This way your branch should be up-to-date. If you need help, let me know.
If you are only using the web interface for github then this will work as
well for updating
http://www.hpique.com/2013/09/updating-a-fork-directly-from-github/
On 4 June 2016 at 11:05, RolandD notifications@github.com wrote:
@uglyeoin https://github.com/uglyeoin I have a PR ready for you but
your staging branch is very outdated. First you will need to update the
staging branch before I can send it to you. You will need to do the
following if you use git on the command line:git pull upstream staging
git pushThis way your branch should be up-to-date. If you need help, let me know.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10011 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABPH8Z7rx042VvmJy6wMP8imXFCyvxw3ks5qIU3ygaJpZM4IL6PB
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
@roland-d (& co) I figured I would try to get used to using Git Bash as opposed to always using the browser version. I have managed to pull uglyeoin/joomla-cms.
Then I tried git pull upstream staging. I got the error:
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.
Having Googled around I see people offering advice of use:
git fetch origin
Does this do the same thing, would this break anything?
@uglyeoin It looks like you have not defined the upstream yet. To define the upstream you can run this command:
git remote add upstream https://github.com/joomla/joomla-cms.git
After that you can do the other 2 commands I posted earlier.
The get fetch origin only pulls from your own repository and not the Joomla one, so that won't help in this case.
Thanks @roland-d , I had worked it out actually it's at the bottom of the docs. Even so, thanks for the quick response it is much appreciated. https://docs.joomla.org/Working_with_git_and_github
However, now I have 2 conflicts.
CONFLICT (content): Merge conflict in modules/mod_articles_popular/tmpl/default. php
CONFLICT (content): Merge conflict in components/com_tags/views/tag/tmpl/default
I assume this means someone else has also edited these files at the same time. And I assume that means I need to manually check what they did/what I did and somehow make those files the same (assuming they haven't changed the code in a way that conflicts with mine).
How can I find out who the other person is and see their version?
It's ok I've worked it out.
I made the changes so the files were the same (someone added in itemprop article - nice addition), I did git push origin. Why does it still say this branch has conflicts? Also when I look at files changed they don't seem to have changed.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
@roland-d I hadn't committed and now I have done so and it pushed properly. This has been a fantastic learning experience, thanks for your help. Once you have merged your changes I will download and test. I suppose that means you need to tell me what the changes are so I can write clear testing instructions for everyone.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
Ok thank you all for your comments, I'm going to test it now.
Upon testing I find no way to set a a featured tag @brianteeman, is this correct or am I missing something? If this feature does not exist then I will remove these files from the pull request.
A contact is set as featured directly in the contact item. Open a contact
and you will see a featured yes/no option.
A contact is set as featured directly in the contact item. Open a contact
and you will see a featured yes/no option.
Yup found it cheers.
As for tags you cannot have a featured tag - I think you are confusing that with the need for your featured css to be present in the tags view as that can also be used to display articles and contacts which night be featured (this was explained earlier in this thread)
Ah, I see, got it. Will review, cheers.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
| Title |
|
||||||
| Title |
|
||||||
| Title |
|
||||||
| Title |
|
||||||
@wojsmol Thanks, I tried doing a pull upstream but I guess it didn't work. Is there a correct way to achieve what I wanted i.e. remove those files from the pull request? I changed them but I didn't need to.
I have copied and pasted them for now. Thanks for the heads up.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
This PR has received new commits.
CC: @richard67, @smz
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
I have tested this item
I tested this and I see the class. I tested it locally on all menu items and modules. If it is easier I will upload the local site online for people to see, but I assume the correct way to test is to do it yourself from scratch.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
You can't test your own code. We assume if you submit it then it works :)
Ah I see, that makes sense. A wild assumption of course in this case :)
working on testing this, but I'm not seeing any new featured item class in the category list view.
Thanks @brianpeat. The array is not passing the featured part, so there's no way to listen for it (if that's the correct way of saying it). I will try to get that part working at some point.
So should I hold off on testing, or do you want to try to move forward with out that view and just say it's just the way it is?
@brianpeat good question. I want to try and fix the issue you have raised. I'm not sure if I have the skills but I will give it a go.
The other bits all work ok, so technically you could test them, but it would make more sense to wait if you haven't already tested them. I'm not going to change those files but it is still more prudent to test the final version, and I don't want to waste your time testing things twice.
Thanks for taking the time to test my patch so far, I will let you know after I do the update or ask for help if I get stuck.
Is this PR going to be completed?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
| Status | Pending | ⇒ | Information Required |
I have every intention of completing it but I have come to a gap in my knowledge. It works on everything except one of the views. That view does not have the "featured" element in the params output. Can anyone give me a pointer to how I can get that information to output, or otherwise call it? Happy to do some learning but I'm not quite sure where to start, I don't believe there is anything to edit in the PHP files I have seen, so perhaps I need to look elsewhere?
Which view?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10011.
The category list view
@brianteeman Any ideas on how to get params output of "category list view"?
@JoshuaLewis I can get the params output, but it doesn't include the featured item within it. So what I need to do is either add it to params, or call it separately. Neither of which I know how to do. @brianteeman or @wilsonge I know you're busy but if you could spare me five minutes at some point I can't make JUGL this week though.
uglyeoin#2 Bon Chance :)
| Category | Components Feature Request Templates (site) | ⇒ | Front End Components Tags Modules Templates (site) Feature Request |
Just checking in on this.
@JoshuaLewis I need to resolve more conflicts and get another test done. I tested it myself which is (obviously) against the rules. Let me fix the conflicts and put out a request for testers again.
Hoping the conflicts get fixed. Would love to see the featured class added. This would enable us to indicate if an article has been featured or not.
If this PR get no Response, it will be closed at 22th June 2017.
Sorry I had fixed the conflicts but more arose, I'll do it again before the marked deadline. I would be grateful for anyone willing to test it after I've made the changes, that's what stopped the initial pull request. I will try to find my own testers too. @JoshuaLewis would you help?
Happy to test this.
Thanks @Bodge-IT & @JoshuaLewis
| Category | Components Feature Request Templates (site) Front End com_tags Modules | ⇒ | Front End com_contact com_content com_tags Modules Templates (site) Components Feature Request |
Ok guys I am going to test now if you could all do the same. @Bodge-IT @JoshuaLewis the testing instructions above should still be relevant. It has been a long time since I coded this, please be vigilant. Please test BEFORE you install the patch that the issue is still an issue. Then test afterwards to see that it is fixed. Please make sure that .item-featured is not appearing on non-featured items too.
com_contacts
com_content
com_tags
have all been affected.
Conflicts don't look good from here
@wilsonge thanks for the info. @Bodge-IT @JoshuaLewis please ignore my test request until I resolve
ping @wilsonge
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10011.
I am available this week to get this resolved if there are issues
It hasn't worked (you can look at the code changes in github to see pretty easily https://github.com/joomla/joomla-cms/pull/10011/files) also it's now conflicting again :)
Is there a way to remove the tags/ default_item from this pull request so it stays synchronised?
I made a change, and I see no "resolve conflicts button". I'm not so hot on this stuff, so would appreciate any assistance to help me get over the line. I'll find some testers but I need to get it into the right condition first.
That's because there aren't any
@Bodge-IT @JoshuaLewis can you test please? Patchtester video if you need it, or add me on skype square-balloon and I'll walk you through testing. https://youtu.be/4OWgusZgIfk I'll try to find some others too, thanks for the assistance.
why is com_tags being changed
@brianteeman at your request, although I would rather remove all tags from this pull request because I'm not sure the necessity is there for featured as they are filtering by tag so it's debatable as to whether this is necessary on this view.
I don't know how to remove the tags files from this pull request, but if anyone can help me with that it would be appreciated. It is also more complicated as tags doesn't pull the menu params which includes featured.
Probably all the tags views will need updating as well
@Quy 's file suggestions are made. @brianteeman 's double quotes query has been changed to singles. I am now going to go through the files again and see if I can spot any other errors such as these.
Let me rephrase that - why are you editing the tags component with changes that are unrelated to featured things
@brianteeman sorry the last question I didn't realise was on a tags file. Please ignore. That would be an entirely different pull request. My ideal scenario is that all tags parts of this PR are removed. I tried resetting them back to the master but obviously it will keep going out of sync. Is there a way to remove just those files?
If someone can remove the tags parts of this PR then I can test it and if happy others can test it. Unless you absolutely want them in, in which case I would still rather do that as a separate PR as the master keeps going out of sync and I don't want this PR to be eternal.
A Month after latest Comment: How to go on with this Pull Request?
If this PR get no Response, it will be closed at 17th September 2017.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10011.
I need to find someone who can help me to remove the tags files from the pull and then get it tested. I'll start looking a little harder for help with that side of things. Give me 2 weeks to get that done.
If file in your branch is equal to joomla-cms staging branch then this file in gone from PR.
+1 for this. As a designer, I totally agree that this would be very useful, and it's what I would expect to find in the code. I expect there to be a class for me to use, even if styling isn't the primary function of the "Featured" feature.
Will be testing as soon as I can.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10011.
What's the status?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10011.
@JoshuaLewis I'm struggling with GitHub as I use it too infrequently. Something I will change moving forwards. For now does anyone know why the commit button is greyed out/disabled? https://drive.google.com/file/d/0B0YuO0L9wjgja3hnSjg5WDdIZ28/view?usp=drivesdk I'm trying to revert the tags files back to the core by copying/pasting the core code.
@wojsmol I'm sure you are right. I clicked on changed files at the top of this conversation, then clicked on the pencil next to the file I wanted to change. I'm aware it's easier to do this in the command line but I am keen to get this one over the line. is it the correct action? It seems to say that this branch has no conflicts with the base branch now too. Does that mean it is somehow fixed? Possibly because of my past attempts?
@wojsmol first of all thank you so much for taking the time to help me and I apologise for my painful lack of experience. I think I am already of staging https://drive.google.com/file/d/0B0YuO0L9wjgjVlRINXgxWFlSZXM/view?usp=drivesdk but perhaps it is my lack of knowledge and I am incorrect.
@uglyeoin See PR uglyeoin#3
@JoshuaLewis @davidsteltz @brianpeat @richard67 @C-Lodder @Bodge-IT call for testers please as per the instructions at the top. If you need any help from me please tag me. You don't all need to test obviously the first two will be fine and I am grateful to anyone who takes the time to help me with testing.
@brianteeman thanks, good catch. Have done so now.
Will revert my test and will say I really dont see the point in this. I feel this is being added for personal preference rather than an actual mass need for it.
That fact a class is being added to the DOM without the core applying any CSS to it, I find absolutely pointless.
Joomla supports overrides, so let users use them
As a designer, I appreciate classes like this being in there. It's a HECK of a lot easier/faster if they exist and I don't have to override a bunch of template files. All I need to do is a few lines of CSS in my template and I'm good.
@C-Lodder I agree with @brianpeat there's a time when I would have had no idea how to achieve this as I didn't know any PHP but I knew CSS/HTML and I knew how to design using other tools like InDesign and Photoshop. It's a totally different skillset.
Besides which, enough people have shown an interest for me to believe that others agree with this being useful. It would have been nicer to state your case as strongly as you could earlier as I've put a lot of work in here.
Happy to debate if you have any strong points but this is not a whole lot of extra code, it adds a feature some people find useful, and it doesn't break backwards compatibility.
wow, 103 commits in 1 pr ... is this world record?
@richard67 now in par with #7435 at 104 comments
Besides which, enough people have shown an interest for me to believe that others agree with this being useful. It would have been nicer to state your case as strongly as you could earlier as I've put a lot of work in here.
For the record many of us did express that we were not in favour of this
Also just because there has been a pull request doesnt mean that it will be accepted. It often happens - you only have to look at last years GSOC projects
@brianteeman understood on both counts. And @C-Lodder is welcome to revoice his concern I guess. I'm really not experienced enough to know how it works. But perhaps in the future the discussion should take place, it should be decided whether it would be accepted and then the person can do or not do the work.
I'm sure we can all agree that we like people to contribute, my concern is allowing them to fix all of the requested amendments and then rejecting it is a bit harsh. If it was rejected at an earlier stage it would have saved me time. We did not need it in perfect working order to know whether it would be accepted.
@richard67 for such a simple change it is a rather embarrassing and unwanted record :)
@brianpeat @uglyeoin If I thought your way, then the entire system would be clustered with a load more classes for every other scenario I've come across.
Requiring a class in a HMTL is not mandatory, but a requirement for a specific user, such as yourself. If you have specific requirements, then there are very simple docs that show you how to create a temolate override.
Is these sort of PR's get merged, then more will most likely be submitted, resulting in a bulkier DOM and those that actually care Bout performance will have to remove them all
@C-Lodder you're assuming I don't think your way. You're assuming I think an additional class should be added for every single scenario. I agree with your stance, I just don't agree in this case. I believe that the featured function should mean something is featured, but it has nothing at all to differentiate it. My stance is above so I won't go into detail. Alternately the featured function could be dropped altogether in J4. That would seemingly fit your stance as it reduces the code.
But I can't agree that it's an individual use case because others have commented saying they want the feature so I think your stance is refutable.
@richard67 100+ commits shows my "commitment"
Heck, I'd go so far as to suggest the default template should actually visibly reflect this change so that if someone is testing the stock install and they feature an article, they can SEE that somewhere (such as a star icon or a bolder title). Right now the only thing you can do with Featured articles (in a stock setup) is to display ONLY featured articles in their own component view, or use a module to display them. The system doesn't do anything to highlight them in a display with other items as far as I know.
All this does is make it a lot easier for a designer to style them so they DO stand out. I don't see what the big deal is honestly.
I didnt say drop the featured function completely. Im simply saying this should be done as an override rather than adding it by default, if we're not actually going to even use the class.
There will always be thing people dont know how to do on Joomla, such as performing an override, or even what cose to add in the override. This is why we have a forum and JSE, to get help
And my suggestion is, then lets use the class. Lets add some tiny css line that does SOMETHING to highlight a featured item in the blog view or other views. Once it's there, lets use it so users trying out joomla actually SEE something there when they choose to feature an article.
@brianpeat be my guest
@uglyeoin I see the point of making front end developers life easier. I also see the point of @C-Lodder that this is bloated code.
The actual problem is not the class that you want to add, it's a bit deeper than that.
Joomla has a very clever overriding mechanism that should be sufficient for anyone that can read and write html to do whatever they want with their site. The problem is the GUI part for that is not a candidate for any design or UX award and besides that it is placed n in a very well hidden area that most users don't even know that it exists.
Fix that and you fix a lot more than a class
But perhaps in the future the discussion should take place, it should be decided whether it would be accepted and then the person can do or not do the work.
You can open an RFC issue, or if you have code to back a proposal a PR with "RFC" in the title as a means to gather interest or open debate. We've had plenty of those over the years.
Ultimately though, even if a proposal gets backing/support, that doesn't mean the PR for it gets merged either if there are issues with the technical implementation. And as pointed out, even if it works, addresses an issue, fills a real life use case, or anything like that, it doesn't necessarily mean the project will accept it (we really do try to be accommodating and not completely reject people's ideas or work, even if it's not something the people heavily working on the core code or the project teams are actively focusing on).
@mbabker Ah RFC = request for comments? Thank you, I will remember that in the future. Although it doesn't seem like I needed that in the title in this case ha ha. But yes it is something I will consider in the future. And yes, understood that not everything gets accepted. If that is the case in this instance then so be it, there's a lot of much more experienced coders/Joomlers who understand these things. I can only put my case across in the end I'm sure their decision will never hurt the project which is fine by me.
@C-Lodder if I use the class somewhere in the default template then I may break some peoples websites so I can't really see how I can do this without causing a much bigger argument and one that I would agree with. This isn't about making the class available for me, it's about making it available for all the template providers out there. I think if it's publicised they will use it. Although that is speculation and I'm not sure if any of them are in on this discussion. People who use K2 will be familiar with it as it is in the core of K2 and is used in templates that support K2.
it's about making it available for all the template providers out there
@uglyeoin sorry that's not totally accurate, since most templates have their own overrides so whatever you might add in the core it will be ignored in the override...
Only case that this will have an effect is for templates that DO NOT ship overrides and there are not so many out there that fall into this category...
By adding it I cant see exactly how you are going to break peoples sites. The only downside is if they have a template override already, they wont have the class unless they redo the override.
Thats the downfall of overrides unfortunately
@dgt41 and any new templates.
@C-Lodder yes but there's nothing I can do about that. If someone has a blog and it has featured articles and I add CSS to add a border round it then they will suddenly see a difference in their blog that was not there before. Assuming they have not created any overrides.
Exploring your idea that if overrides exist a change won't take place. It's not like we decided not to include the schema markup in the core because people may have overrides and then it would be ignored. There must be examples of changing the core that won't be implemented when overrides already exist. I don't see any precedent for that stance. The router is a great example although not visual, older sites may break if they use it but that does not mean it should not be included.
If I understand this correctly, If I want this now I have to make an override, learn how to poll the params, check to see if an article is featured, and write a class if it is, all in my override.
With this pull request, I can still do an override for stuff, but I don't need to know php, I can literally just use CSS to do what I want. To me that's huge. I might actually USE the featured articles function for once. Sure, eventually I should learn how to pull params, but why should I if all I want is to style a core function that's already there? That's why I like this idea.
@dgt41 I guess that could work, but as it doesn't create a b/c then we could include it in 3.x and just do the template change for J4. Besides it only really solves one of the counter arguments.
If I am going to do that then I would do an RFC to see whether people think the featured function should be removed or remain for J4.
At the moment people can only use it via a menu item I would be interested in seeing how many people use that menu item. I never use it and haven't since 1.5 when, as Brian Teeman pointed out, it was common place to use it to create a homepage.
I do not create homepages like that any more but if the sole purpose is to create a page with certain articles in it then I think featured is the wrong name and if we keep it the UX team should be involved in the decision. For me that opens a new discussion. Which may seem strange as I'm the one who sees the purpose of this pull request. But if a feature is rarely used then removing it from core can save code which seems to be what the devs in the conversation want (and I can understand that).
The main reason the featured class would be useful for many is articles could indicate if it was featured or not. A well organized Joomla site will often use menu items for each category (as opposed to simply viewing through featured articles), so when articles from a specific category are viewed it would be nice for admins to be able to indicate that a noteworthy article was featured. Non admin users will appreciate this by knowing which of their posts have been featured or not.
| Status | Information Required | ⇒ | Discussion |
as this Pull Request got response set Status back on "Discussion".
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/10011.
Given that the featured state is technically a status, it makes sense to have classes associated with an article status. Take for example when editing an article it's parent DOM element has an edit class for styling a view that is in a specific state. Putting styling aside, an edit class allows us to place text anywhere we want with CSS or JS making it extremely easy for people to add relevant information in accordance to the article status.
I agree that we don't want to load up tons of classes in every scenario of Joomla. However it's reasonable to have a few classes such as Unpublished, Deleted, and Featured which are very common article statuses through out Joomla communities. This is more than just indication/styling, giving people the right info or formatting to more easily do specific user interactions benefits viewership and contributors.
I'm aware that eventually this pull will go out of sync again. What is the correct process for finding out whether this will be accepted or not? Who decides and what parameters do we have to meet if any? There's no point in discussing this forevermore if it won't be accepted. Or is the discussion the next step, in which case with neither side agreeing, what is the best process to finish the debate? Should I list out all the objectives/pros/cons for each side and address each? Would it help to show some visual examples of the featured state being styled? Would it be helpful to show other websites as examples?
It's been 22 days without comment. I don't know who the correct person to ask is. So I'm picking some people who I know are knowledgeable and understand the system well @mbabker @wilsonge @brianteeman can you help draw this long winded pull request to a conclusion? Or direct me to the appropriate person.
As you ask my view is here #10011 (comment) but i am not the one to make a decision
@brianteeman your view, as is everyone else's is very much appreciated and adds to my learning. Do you know who is responsible for the decision (either person or team?)
@uglyeoin Release Lead decide.
There is also a conflicting File.
Thanks @franz-wohlkoenig looks like Michael. I've already tagged him, so I guess nothing else for me to do at the moment. Will fix the conflict if the pull is going to be accepted.
Michael is release lead for 3.8 series, Im cms release lead which means testing and reporting if there is a no show problem
@flyingwombats thanks Phil
286 comments later, what's happening with this?
It's PR decisions like this that are the straw that break the camel's back for Joomla. 10 Years ago I was extremely proud of the achievements that Joomla was running with, even 5 years ago it was still running strong. Awesome folks like @uglyeoin come in with good innovations that support existing Joomla features only to be halted by grouchy administration. I know it's not the PR thing to say, but Joomla has been over ran by bureaucrats and is giving other CMS's the progressive software market. Joomla will still stand but not to what it could have been.
And yes this is my resignation from the Joomla community. It's been a lot of fun, met a lot of great folks, and there have been a lot of great administration in the Joomla staff as well. Had one member as a college teacher.
@JoshuaLewis (sorry on my phone) see above
You made a great PR, never regret that nor should you let the community make you feel that way. Unlike other people who leave things behind, I'm a forgiving lad. ;-) I may take a peek on occasion, seek out passionate developers from the Joomla world, and come back if I see that Joomla take progression more seriously in the upcoming years.
Also note that it's been a long list of things in the making. The biggest issue for me is performance on large scale community projects even with multiple data caching/compression, reducing extensions, and doing dozens of other tricks in the book including using a good host.
Reading this again, my opinion.
-
The core workflow in Joomla for featured content is to mark content for use in a special view outside the content hierarchy, with options added to an option stuffed interface to use this special state in other scenarios
-
To me it goes counter to the core workflow to introduce DOM classes in core (used or not, and if used this would actually be more disruptive because it changes existing styles) to style some content based on publish state or certain flags in the item's config (like featured) and in effect change the workflow for this specific state/flag.
-
While there may be valid use cases for it, in my best judgment this is not just a simple change to the rendered DOM but a philosophical and B/C breaking workflow change that IMO warrants a proper discussion about whether this belongs in core or not, and to be honest I doubt there is going to be a consensus either way.
I understand the core workflow, maybe it could be renamed selected articles. Maybe it could be removed altogether, the same thing can be achieved with modules I believe. So one less bit of code to maintain.
The people for the pull request are mainly so because of the word “featured”. I know that renaming it in itsself requires a ridiculously long discussion, but if we can never change things we’ll never progress. Users will always find Joomla! confusing.
In the event of no concensus do we just leave the discussion bubbling along forever? Does someone make a final decision? Was that a final decision? Who needs to be involved in the discussion? I’m asking because in the future I don’t want to spend so much time on a simple pull request without knowing the correct procedure:
It should probably be documented somewhere. It’s a little unfair to expect an attempted contributor to keep their pull request in sync with the core for 2 years.
Now that you talk about renaming the "feature" I understand your use case a little more.
Some history first. Originally this was called "frontpage" and its intention was to allow you to create a front page on your web site with selected articles from many different categories. It was later changed to "featured" as you could create a page like this anywhere on the site and not just on the "frontpage".
The object of the "feature" is to allow you to display selected articles from many different categories on the same page. It was not the objective to allow you to highlight an article within a category, on a category blog for example, This sounds like what this PR is trying to achieve. (?)
This PR is trying to achieve the potential of highlighting an article. Although there's definitely a use case for choosing articles from different categories on a page.
The main reason is because they are called featured, so usually if something is featured somewhere it is different from other content. Much in the same way I would be raising a pull request for intro articles if they appeared at the bottom of the page instead of as introductory articles.
The name is confusing. I've used Joomla! a long time, I remember this being the favoured way to create a front page. I no longer use this method, but others may, I don't know. But let's be honest, so many people have not been using Joomla! as long as you, and I think newcomers will find this confusing and expect it to work in the way that I have created in this pull request.
My personal opinion is that if I need articles from multiple categories on a page then there's a decent chance I will need more than one page with that set up. I'm not sure if this is possible in the core at the moment but it would be with extensions.
So keeping it as it is, I think requires a rename to stop confusion. A rework so it can work on multiple pages. Or removal if it is not a commonly used feature.
I know there was an uproar at the stats collection feature, but I think an optional plugin giving Joomla! feedback about what menu items and settings are being used would be really helpful as we would then know how much this feature is used.
If it's going to stay as featured, then I think my pull request still makes sense, it doesn't detract from the original purpose. But I think the debate has been done, so I have no idea where to go from here. Unless Michaels decision is a final one, in which case, we don't really need to discuss it at all.
The main reason is because they are called featured, so usually if something is featured somewhere it is different from other content
Yes it is different - it can be displayed on the featured items page
My personal opinion is that if I need articles from multiple categories on a page then there's a decent chance I will need more than one page with that set up. I'm not sure if this is possible in the core at the moment but it would be with extensions.
This is already possible with the core code - no extensions required
So keeping it as it is, I think requires a rename to stop confusion.
Open to suggestions to a rename although since the name was changed from frontpage to featured in june 2009 I haven't seen any comment about the name being confusing before.
Unless Michaels decision is a final one, in which case, we don't really need to discuss it at all.
Not right now it's not. But, to me it's clear accepting this PR introduces a philosophical workflow change in how featured items in core are supported. Yes, users may already be using this feature in a way that this PR supports, but it's not really a core supported feature/workflow. No, we could not introduce styling rules to the core templates before 4.0 in support of this feature/workflow as sites using Protostar or Beez3 would have their displays altered in what is probably an unwanted manner (and even then, if we did that then it kind of forces this approach to be used at all times).
I'm not totally opposed to this FWIW. But, at a higher level, to me there is a lot more to consider than just adding some classes to the rendered DOM for exactly the reasons I pointed out.
for reference there is a similar issue regarding archived articles
It all depends on what core wants to define featured item workflow as. My opinion hasn't changed from before, my issue isn't the proposal itself at this point but that it changes the fundamentals of what we define featured as and how integrators should work with it; right now I lean toward declining if we stick to what featured is currently defined as but if we want to change it to match these types of use cases that exist in the ecosystem then merge.
I'm not really sure why the workflow thing is such an issue "even if unused". I just means there are multiple ways to use the same button/feature or whatever you want to call it. You can use it the old way and be unaffected. If it's just a philosophical thing, then everyone can have a different opinion already. For example some people use it for a homepage and some use it for a featured page. I don't use the featured menu item at all. So already there are three philosophies and knowing this community there are bound to be even more. Joomla! is full of people using their own workflows for things.
Can you maybe explain it in a different way so I can understand where you're coming from?
Can you maybe explain it in a different way so I can understand where you're coming from?
#10011 (comment) and #10011 (comment) cover it at a high level.
I'm of the opinion that the core code should follow some sort of specification (this is what a feature was designed for, these are the use cases that we will support as a result of it; said specification should turn into a set of automated tests to validate it). Lacking a defined specification, I fall back onto how the code is designed and what the workflow is in core (and those two comments cover what I feel is the use case for featured items and inherently what the feature specification should be). The fact that users are able to use this feature in ways that core didn't design for is pretty cool, but that doesn't necessarily mean that core has to support all of those use cases either.
Fundamentally, I do believe that accepting this pull request means that core accepts this use case as a "core supported" workflow for featured items, which is a change from the existing implementation/workflow of featured items (featured items go from "only" something which is in essence a special state used to quickly group things onto a special view to items which can also be highlighted in their own way site wide). I'm not opposed to that type of workflow or feature, like I said I think it's cool that users are able to use core features in ways they weren't designed for. That's why for me accepting this PR does introduce a philosophical change, it changes the feature specification (if one existed, we don't have those for most things in core sadly).
I am personnaly in favour of core supporting both cases, as both make sense.
To be honest I didn't read the 300 comments but as Michael wrote, this is not simply adding classes to template files.
Personal I miss use the feature attribute of the articles too but I do this in template overrides and that can be done based on the use case. I also see a small information leak adding the feature class to the html tag.
Anyway I will not merge this in to J3 series and maybe it can be considered for J4 but now I would like to ask you @uglyeoin to rebase this PR on J4 or create a new issue for discussion to add this in J4.
Thx for you time. In the meantime I close this PR.
| Status | Discussion | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-06-29 22:04:54 |
| Closed_By | ⇒ | HLeithner |
@HLeithner I don't think anyone would blame you for not reading the 300 comments. It's for that same reason I'll leave this closed and not rebase to J4. I never quite got my head around @mbabker s point but hey ho, I'll leave it parked here.
TL;DR adding classes to the DOM changes how featured items are used in core. That's why I didn't agree with just blindly merging this PR 2 years ago even though it does exactly what it advertises without any real side effects in the state it's in now. And as I said in #10011 (comment) if there were a feature specification (LOL) then this pull request changes that feature specification.
Nothing stops you or anyone from applying this change in your sites. But that doesn't necessarily mean this change is right for all Joomla sites.
Without wanting to start a full debate again, but just to learn from your much higher knowledge than mine. It feels a bit like you're saying "best practice is to write a feature specification and then build the feature. This didn't happen in this case, and in practice there's no side effects." So it's kind of like you disagree with it from a theoretical point of view?
Since there isn't a feature spec, I fall back onto what the code currently does. And based on my interpretation of the code, the featured flag isn't intended to be used on the frontend to style content in a distinctive manner like what this PR would allow to do out-of-the-box (it comes across to me as a feature to be used for marking highlighted content to be displayed on a special view, not as a feature that should be used for marking highlighted content site-wide). So that's why I believe this PR changes the definition of a featured item.
It may very well be there are a number of site owners who do want featured items to behave in the "highlight this content wherever it is displayed on the site" way, clearly that's possible now because all this PR is doing is adding classes to layouts (it's not altering any queries or other PHP code). So as long as someone is maintaining their site and using featured items in this way consistently, then it's no problem to use featured items on your own sites in this way. But, I don't think that the core definition of featured items needs to change as a result. We use the featured flag on various joomla.org sites to ensure key recent articles are highlighted and generally have the homepages set up as a featured item view, this gives us full control over what posts are visible on the homepage and what posts are only visible in their respective category feeds, with this PR in core we would have to be cautious about any CSS changes leaking into the template and changing the display of these items site-wide.
You should change
<div class="leading-<?php echo $leadingcount; ?><?php echo $item->state == 0 ? ' system-unpublished' : null; ?> <?php echo ($item->featured) ? 'featured' : ''; ?>"to
<div class="leading-<?php echo $leadingcount; ?><?php echo $item->state == 0 ? ' system-unpublished' : null; ?><?php echo ($item->featured) ? ' featured' : ''; ?>"i.e. remove the space before "<?php echo ($item->featured) ? 'featured' : ''; ?>" and add a space to the beginning of 'featured'.
Otherwise, if you don't change that, you will have 1 space at the end of the class attribute if the article is not featured.