-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simple theme fixes #2976
Simple theme fixes #2976
Conversation
In the simple theme, some templates are using `h1`, others are using `h2` for the main title of the page (other than the one in the header). This commit changes that so all of the pages are using `h1`.
Add a <main> tag to surround all the content blocks, so that it's easier to target the main part of a page (be it an article, the list of posts or the different categories) using CSS. Because of this, the <section> part of the article.html template is made redundant, so it is removed. Finally, the generic <div> is replaced by an <article> tag to surround the article's content.
LGTM. |
I think it's already using a lot of HTML5 tags ( One comment I have is that the templates are using a lot of <body id="index" class="home">
<header id="banner" class="body">
(...)
<footer id="contentinfo" class="body"> I don't really see the point of the |
I agree with your perplexity about the presence of a lot of |
So do you think I should rework this as well? I can trim a lot of things, since a lot of tags can be targeted by a specific CSS rule without having to rely on a specific |
Yes, from my point of view, but I think it would be better to seek the advice of other core developers |
OK, then I'll wait for others to comment on this and take action, if any :) |
I went ahead and made some modifications to the |
LGTM |
All of the modified HTML tags can be accessed in CSS without the need for a dedicated id or an additional class.
ab58bdc
to
16b8a03
Compare
I've rebased this branch onto master. Not sure if this can be merged now? |
<header> | ||
<h2 class="entry-title"> | ||
<h1 class="entry-title"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're moving simple theme to a classless theme we can remove also this class and all the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to leave this class because in the base.html
also has an <h1>
tag within a <header>
tag, so I thought it would still be easier to target the entry title using a dedicated class in CSS. What do you think?
Thanks for the enhancements and your patience, Pierre. Much appreciated! Kudos also to @pauloxnet for the help with reviewing. 👏 |
</header><!-- /#banner --> | ||
<nav id="menu"><ul> | ||
</header> | ||
<nav><ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes like this one will break websites that extend the Simple
theme; what was the motivation?
Many websites extend the Simple theme and add their own CSS; removing these will break them. Was anything gained in the removal? |
@cpitclaudel: Avoiding disruption for existing users is a priority for us. Rather than speaking in theoretical terms, could you point us to specific sites that have been affected by this change? |
A set of changes for the
simple
theme to be mobile-friendly and to useh1
consistently throughout the template files. Please check individual commit messages for more info.