-
Notifications
You must be signed in to change notification settings - Fork 76
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
Improvement: Rework Hn Structure #524
Conversation
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.
Hi @tblivet, I love fixing the semantic strucutre. ❤️
What I don't agree with is adding the new first-title
to sixth-title
. In my opinion, it only brings extra data to transfer and h1
to h6
classes are understandable enough.
Hi @Hlavtox 🙂 I agree over the fact that it brings extra data, the initial approach was to separate the semantic Hn naming and the CSS class naming to avoid this type of code : |
Hello, I understand your concerns, it is true that a lot of CSS variables have been added in this PR, however from my point of view this makes it easier to modify the theme settings by any dev without having to recompile what is really an asset and a good practice! |
Hi, I also don't like the idea to have more unneeded classes for headings. |
@@ -18,9 +18,18 @@ | |||
role = "tabpanel" | |||
> | |||
<div class="step__title js-step-title"> | |||
<h1 class="step__title-left h3"> | |||
{if isset($step_is_current) && $step_is_current} |
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 think it always exists so you can do this:
{if isset($step_is_current) && $step_is_current} | |
{if $step_is_current eq true} |
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.
Great job @tblivet and thanks for listening to the feedback from the community :) I added one suggestion
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.
thanks @tblivet 👏🏻 |
.h4
on a<h2>
tag for example that can look confusing. In another hand, those modifications will help theme devs to edit global titles styles in an easier way.