Skip to content
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

Merged
merged 28 commits into from
Jun 30, 2023

Conversation

tblivet
Copy link
Contributor

@tblivet tblivet commented Jun 27, 2023

Questions Answers
Description? This PR has for objective to improve the Hn structure of hummingbird and ensure a harmonization of the different titles styles by adding specific class on each title instead of using Bootstrap class like .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.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? --
Sponsor company @PrestaShopCorp
How to test? You can check for the Hn stucture of the main FO pages and ensure we have a good structure like H1 → H2 → H3 instead of H3 → H1 → H4 for example.

@tblivet tblivet changed the title Rework hn semantic Improvement: Rework Hn Structure Jun 27, 2023
Copy link
Contributor

@Hlavtox Hlavtox left a 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.

@tblivet
Copy link
Contributor Author

tblivet commented Jun 27, 2023

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 : <h1 class="h3"> who in my opinion can be a bit strange.
But I think, it's more related to a habit than a good practice, so do we agree that I have to do the same but using the Hn classes ?

@ga-devfront
Copy link
Collaborator

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!

ga-devfront
ga-devfront previously approved these changes Jun 28, 2023
@Oksydan
Copy link
Contributor

Oksydan commented Jun 28, 2023

Hi,

I also don't like the idea to have more unneeded classes for headings.
We are using BS5 and we should stick to it's typography classes. It should be easier for module developers to transition their solutions to hummingbird and style modules in general. We should follow one guidelines and don't reinvent the wheel. For new developers that are familiar with Bootstrap it will be also easier to add new functionalities w/o reading that whole css files to grab right class for heading.

@@ -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}
Copy link
Contributor

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:

Suggested change
{if isset($step_is_current) && $step_is_current}
{if $step_is_current eq true}

Copy link
Contributor

@kpodemski kpodemski left a 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

@tblivet tblivet requested a review from kpodemski June 30, 2023 07:13
@florine2623 florine2623 self-assigned this Jun 30, 2023
Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @tblivet

Thanks for your PR, I tested it and all LGTM ✔️

Thanks!

@kpodemski kpodemski added this to the Beta milestone Jun 30, 2023
@kpodemski kpodemski merged commit 1a9638c into PrestaShop:develop Jun 30, 2023
6 checks passed
@kpodemski
Copy link
Contributor

thanks @tblivet 👏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants