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

Templates: fix formatting #593

Closed
wants to merge 5 commits into from
Closed

Conversation

yannicka
Copy link
Contributor

@yannicka yannicka commented Feb 9, 2024

Questions Answers
Description? Currently there is no consistency in formatting. Sometimes there are two spaces, sometimes four; sometimes the tags are not aligned; sometimes single quotes are used, sometimes double quotes; sometimes there are spaces between tags, sometimes not, etc. The goal of this PR is to improve the situation, even if there is still a lot to do.
Type? refactor
BC breaks? no
Deprecations? no
Fixed ticket? N/A
Sponsor company N/A
How to test? Install the theme and browse the website.

@SharakPL
Copy link
Contributor

SharakPL commented Feb 9, 2024

I started something similar, but it's not worth it. You would only trigger lots of new translations for all languages so a lot of your changes are really unnecessary. Sorry

The rest can be fixed along the way while fixing other bugs without triggering updates of 100+ files.

@yannicka
Copy link
Contributor Author

yannicka commented Feb 9, 2024

I've updated only few strings (two or three maybe). So it can be a separate PR without problem.

I don't think it's a problem to have a large PR like this. It helps to set things down a bit. Otherwise, it looks like it will never happen (it never has, even though the theme has been around for years).

I work every day with the PrestaShop theme (currently the "classic" one), and it would be nice to have a well-formatted code base, which makes work more pleasant.

@Hlavtox
Copy link
Contributor

Hlavtox commented Feb 23, 2024

@yannicka Hey, first off, it's very cool to fix all the formatting things, but PR big like this makes it very difficult to review. :(

There are some things I don't like, for example the empty lines everywhere and others, but that could be discussed.

Do you think we can separate the PR into smaller chunks? For example - a PR that will only fix quotes " to '? That would be easy to review and could be merged right away. :-)

Also, you need to rebase it anyway, because some changes were merged.

@yannicka
Copy link
Contributor Author

I'll see about cutting my PR into smaller chunks when I have time:

  • Change of translations.
  • Changes to single quotes/double quotes.
  • Changing long lines to several lines.
  • Add blank lines to space out.
  • Changing indentations.
  • Other small changes.

@yannicka
Copy link
Contributor Author

" to ' done in #601. It's the least interesting change, but also one of the biggest (in terms of differential, not final cleanliness, as the change itself only brings consistency).

@yannicka
Copy link
Contributor Author

Blank lines added to improve readability: #602.

@yannicka
Copy link
Contributor Author

Fix search translation in #603.

@yannicka yannicka closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants