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

Add meta framework into theme #649

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

matks
Copy link
Contributor

@matks matks commented Aug 20, 2024

Questions Answers
Description? Add meta framework into theme to allow module developers to find out what framework is used. See similar PR for Classic PrestaShop/classic-theme#149
Type? new feature
BC breaks? no
Deprecations? no
Fixed ticket? Fixes partially #581
Sponsor company
How to test?

@Hlavtox
Copy link
Contributor

Hlavtox commented Aug 20, 2024

I overlooked it in the second PR, do we want to put it into compatibility section? Isn't it more an general info about the theme?

@kpodemski
Copy link
Contributor

Maybe we could be less specific and use "bootstrap-v5.2" or even "bootstrap-v5", just not to force devs to update every time we update Bootstrap version, my understanding is that they do follow SemVer, so we should be safe.

Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @matks

@jolelievre jolelievre merged commit 13fa0d9 into PrestaShop:develop Aug 20, 2024
6 checks passed
@ps-jarvis
Copy link

PR merged, well done!

Message to @PrestaShop/committers: do not forget to milestone it before the merge.

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.

I'm blocking this PR since we need to be careful not to integrate this flag incorrectly.

It's crucial to have an easy way for devs to know that the theme base on the more modern version of bootstrap, but having to do checks like:

if (v = bootstrap-v5.2 || v = bootstrap-v5.3) would not be efficient, unless we will provide some abstraction layer over this flag, so we can easily parse this:

if (framework.name == bootstrap and framework.version.major == 5)

@kpodemski
Copy link
Contributor

oh no @jolelievre 😅

@matks
Copy link
Contributor Author

matks commented Aug 20, 2024

@kpodemski Don't worry 😄 this is just a merged PR, not a "final act"

  • code can evolve
  • in the last resort, PRs can be reverted

I personally don't have a strong opinion on the content of that property, so every option is OK for me 👍

@matks matks added this to the v0.2.1 milestone Aug 20, 2024
@jolelievre
Copy link
Contributor

Hi @kpodemski

yeah I merged this early because I needed both themes to have the extra configuration to handle the specific modal, but we can always improve those two values before the next release don't worry.

I think having bootstrap-v5.2 is better, I would even go further and integrate the patch version bootstrap-v5.2.5 for example, the reason is because we can use semver comparison method to check this data.

If you look at this PR PrestaShop/PrestaShop#36731 you can see I'm using some JS method to check the version so there's no need to split the version into major/minor/patch It's easier and more accurate to compare the whole version with semver rules like satisfies(frameworkVersion, '^4.0.0')

You can look into the JS library small docs here https://www.npmjs.com/package/compare-versions to see how more versatile it can be (1.2.7 || >=1.2.9 <2.0.0, ^10.0.0, ...).

And if you think we also need this kind of comparison in twig and/or smarty I think we can add some functions to allow similar tests

@kpodemski
Copy link
Contributor

@jolelievre

And if you think we also need this kind of comparison in twig and/or smarty I think we can add some functions to allow similar tests

Yes, that would be great. Ultimately, we should be able to do such comparisons on the controller and view level.

@jolelievre
Copy link
Contributor

@kpodemski we already have this kind of tools in PHP thanks to composer https://github.com/composer/semver/blob/main/src/Semver.php#L32

We'd just need to add a twig and smarty functions to use it in the view

@kpodemski
Copy link
Contributor

@jolelievre ok, I thought we'd like to wrap it, so that we don't rely on composer's code directly 👍🏻

@matks
Copy link
Contributor Author

matks commented Aug 28, 2024

Hi @kpodemski

yeah I merged this early because I needed both themes to have the extra configuration to handle the specific modal, but we can always improve those two values before the next release don't worry.

I think having bootstrap-v5.2 is better, I would even go further and integrate the patch version bootstrap-v5.2.5 for example, the reason is because we can use semver comparison method to check this data.

If you look at this PR PrestaShop/PrestaShop#36731 you can see I'm using some JS method to check the version so there's no need to split the version into major/minor/patch It's easier and more accurate to compare the whole version with semver rules like satisfies(frameworkVersion, '^4.0.0')

You can look into the JS library small docs here https://www.npmjs.com/package/compare-versions to see how more versatile it can be (1.2.7 || >=1.2.9 <2.0.0, ^10.0.0, ...).

And if you think we also need this kind of comparison in twig and/or smarty I think we can add some functions to allow similar tests

New PRs for themes

@matks matks deleted the matks-patch-1 branch August 28, 2024 09:52
@jolelievre
Copy link
Contributor

@matks I don't know if you read my comment, but I don't understand the interest of your PRs?
We have tools that allow checking, comparing versions efficiently and very, so why not give as much detail as possible? 🤔

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.

5 participants