PR-staging ?

Success

User tests: Successful: Unsuccessful:

avatar Kubik-Rubik
Kubik-Rubik
19 Jun 2015

Sets the correct heading tag for the title in the article view if no page heading is provided + code styles

At the moment the h1 tag is used for the page heading and h2 tag for the title in the article view of the content component. But if no page heading is provided, the title still has only the h2 tag and not the h1 tag as it should be.

I also couldn't resist to clean the file a little bit and improve the code style. So, the main change for the fix is in line 42 and line 46.

How to test

  1. Create an article
  2. Create an link to this article in the menu
  3. In the menu entry go to the tab "Page display" and enable the option "Show Page Heading". You should als enter a value in the option "Page Heading" or "Browser Page Title" to see the output in the frontend.
  4. Open the article in the frontend with the link from the menu. You will see that the page heading will have an h1 tag and the article title an h2 tag.
  5. Now go to the menu entry in the backend and disable the option "Show Page Heading".
  6. Reload the article in the frontend and you will see that the page heading is not displayed any more but the article title still has the h2 tag instead of the correct h1 tag.
  7. Apply this PR and check again. The title will have the h1 tag.

Votes

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

avatar Kubik-Rubik Kubik-Rubik - open - 19 Jun 2015
avatar Kubik-Rubik Kubik-Rubik - change - 19 Jun 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Jun 2015
Labels Added: PR-staging
avatar brianteeman
brianteeman - comment - 19 Jun 2015

Is it a problem that this PR will change the H tags on existing sites?

On 19 June 2015 at 11:51, Viktor Vogel notifications@github.com wrote:

Sets the correct heading tag for the title in the article view if no page
heading is provided + code styles

At the moment the h1 tag is used for the page heading and h2 tag for the
title in the article view of the content component. But if no page heading
is provided, the title still has only the h2 tag and not the h1 tag as it
should be.

I also couldn't resist to clean the file a little bit and improve the code
style. So, the main change for the fix is in line 42 and line 46.

How to test

  1. Create an article
  2. Create an link to this article in the menu
  3. In the menu entry go to the tab "Page display" and enable the option "Show Page Heading". You should als enter a value in the option "Page Heading" or "Browser Page Title" to see the output in the frontend.
  4. Open the article in the frontend with the link from the menu. You will see that the page heading will have an h1 tag and the article title an h2 tag.
  5. Now go to the menu entry in the backend and disable the option "Show Page Heading".
  6. Reload the article in the frontend and you will see that the page heading is not displayed any more but the article title still has the h2 tag instead of the correct h1 tag.
  7. Apply this PR and check again. The title will have the h1 tag.

You can view, comment on, or merge this pull request online at:

#7203
Commit Summary

  • Set the correct heading tag for title in the article view if no page heading is provided + code styles
  • code style

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#7203.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar Kubik-Rubik
Kubik-Rubik - comment - 19 Jun 2015

@brianteeman Not a problem but an improvement! :-)

avatar blueforce
blueforce - comment - 19 Jun 2015

Tested on current Version 3.4.1 enviroment with php 5.5.26
Patch works correct without problems.


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

avatar blueforce blueforce - test_item - 19 Jun 2015 - Tested successfully
avatar mbabker
mbabker - comment - 19 Jun 2015

You're using spaces for indents...

avatar Worti2
Worti2 - comment - 19 Jun 2015

@Viktor: In the article I use h3 as next lower title. Is there a new Problem if the title is h1 and the next lower title is h3?

avatar Kubik-Rubik
Kubik-Rubik - comment - 19 Jun 2015

@mbabker Good catch, thanks. Did it on my Mac, still have to optimize some settings there...

@Worti2 Don't think that this is a big problem. Of course you could then update your content and use h2 instead.

avatar Simonkloostra Simonkloostra - test_item - 19 Jun 2015 - Tested successfully
avatar zero-24 zero-24 - change - 19 Jun 2015
Easy No Yes
avatar zero-24 zero-24 - change - 19 Jun 2015
Category Front End
avatar smz
smz - comment - 20 Jun 2015

Sorry to say that (I really hate, as I'm not so much integralist as far as B/C goes), but isn't this a royal breach of B/C? A lot of sites are potentially already styled expecting the old behavior...

avatar Kubik-Rubik
Kubik-Rubik - comment - 21 Jun 2015

@smz Don't see a problem with B/C here. All overrides will still work the same as we don't introduce new variables. Yes, only users without any template overrides will benefit from this change. To make template creator aware of it, we could tag it as a new feature. What do you think?

avatar smz
smz - comment - 21 Jun 2015

@Kubik-Rubik, agreed that there will be no problem if your template uses overrides.

In my templates (I always roll-up my own templates) I normally tend not to use overrides, as much as I can, and I instead rely on a template.css stylesheet for most personalizations. I also tend not to use the "Page Heading" field, as much as I can.

Heading for my articles, thus, is displayed through a css declaration of this kind:

.page-header h2 {whatever-I-like-inside-this}

I might also have a very different style to be used when I want (i.e. the rare times I set the "Page Heading" explicitly for displaying something differently):

.page-header h1 {something-very-different-in-here}

With your PR applied, styling of my articles pages will change, and that why I'm calling this a breach of B/C

I agree that your solution is more semantically correct, but anyway...

avatar smz
smz - comment - 21 Jun 2015

If anyway we want to go on with this (I will not protest, I can fix my stylesheets...), I suggest that we take the occasion of this minor styling B/C breach so that headings deriving from menu-items ("Page Heading") and headings deriving from content title are embedded in two different classes.

Now both are in a .page-header div: we could use .content-header for headings coming from the "Title" attribute...

avatar smz
smz - comment - 21 Jun 2015

... also, when we do not display the content "Title" (Options, Display Title, No) we still generate a <div class="page-header"> with an empty <h2> (or <h1>) tag. Is there any reason for that? This causes issues if for example you are styling div.pag-header with a lower visible border. You wouldn't expect it being there if you don't display the Title...

avatar rdeutz
rdeutz - comment - 23 Jun 2015

The problem with this patch is that it has or can have a visual effect, the site looks different after an update. Fixing this is not a problem for an experienced user and I agree from the semantic it is better, but I wouldn't merge it.

avatar Worti2
Worti2 - comment - 23 Jun 2015

@Robert: You are right with the not so expreienced users. And I try to represent them. But in my opinion this is an issue. Maybe we will change this in Joomla! 4 together with other template changes.

avatar brianteeman
brianteeman - comment - 23 Jun 2015

This break`s existing sites
Imagine a site with the following css

h1 {font-family:serif;size:32px;color:black;}
h2 {font-family:sans;size:16px;color:red;)

This PR completely changes the styling of the site


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

avatar Simonkloostra
Simonkloostra - comment - 23 Jun 2015

I think though indeed this might change existing site's look and feel somewhat (h1's and h2's are usually not that far away from eachother in pixel-size anyway) it does solve an ongoing issue with Joomla's H1-H2 structure. Simply because it was always an issue causing people to fix this in funny ways should not mean that we should stick to those funny solutions.
Also: if people now have an H2-H3 structure it won't be that bad if they then have a H1-H3 structure.

Just my 2 cents ;)

avatar brianteeman
brianteeman - comment - 23 Jun 2015

(h1's and h2's are usually not that far away from eachother in pixel-size anyway)

You have no way of knowing that and as my example shows it could be way more than pixel size

avatar Simonkloostra
Simonkloostra - comment - 23 Jun 2015

Correct, I am speaking in general, there may indeed some for which the difference is more visual.

avatar Bakual
Bakual - comment - 23 Jun 2015

I agree with Brian and Robert here.
I think it's something to consider for 4.0, but not worth the risk of changing the appearance of many sites.
Also this would have to be done in all other places as well, so it's consistent.

avatar zero-24 zero-24 - change - 26 Jun 2015
Status Pending Needs Review
avatar Kubik-Rubik Kubik-Rubik - close - 9 Jul 2015
avatar Kubik-Rubik
Kubik-Rubik - comment - 9 Jul 2015

Okay, since we could produce a B/C problem with this change, I will close this PR.

Who wants to benefit already in the 3.x version, can use this patch: https://github.com/joomla/joomla-cms/pull/7203.patch

avatar Kubik-Rubik Kubik-Rubik - change - 9 Jul 2015
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2015-07-09 20:25:47
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 9 Jul 2015
avatar zero-24 zero-24 - close - 9 Jul 2015
avatar Kubik-Rubik Kubik-Rubik - head_ref_deleted - 9 Jul 2015
avatar zero-24 zero-24 - change - 9 Jul 2015
Labels Added: ?
avatar zero-24
zero-24 - comment - 9 Jul 2015

@Kubik-Rubik i have add the Joomla 4.0 label to re-evaluate this for v4.0 :smile: Thanks

avatar zero-24 zero-24 - change - 24 Aug 2015
Milestone Added: Joomla 4.0
avatar zero-24 zero-24 - change - 24 Aug 2015
Milestone Added: Joomla 4.0

Add a Comment

Login with GitHub to post a comment