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.

Sign up for GitHub

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

Jump to bottom

[invalid] Fix code styles #3561

Closed
wants to merge 81 commits into from
Closed

[invalid] Fix code styles #3561

wants to merge 81 commits into from

Conversation

bjorvack
Copy link
Contributor

Type

  • Enhancement

Pull request description

Fixes most code styles

@bjorvack bjorvack requested a review from a team as a code owner September 20, 2023 11:41
tijsverkoyen
tijsverkoyen reviewed Sep 20, 2023
View reviewed changes
composer.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/Modules/Backend/assets/Backend/webpack/scss/components/_buttons.scss Show resolved Hide resolved
src/Modules/Backend/assets/Backend/webpack/scss/components/_buttons.scss Show resolved Hide resolved
src.fork5/Backend/Core/Engine/DataGrid.php Outdated Show resolved Hide resolved
src.fork5/Backend/Core/Engine/DataGrid.php Outdated Show resolved Hide resolved
carakas
carakas requested changes Sep 20, 2023
View reviewed changes
Copy link
Member

@carakas carakas left a comment

Choose a reason for hiding this comment

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

Did you test all the changes you made? Cause I see a lot of changes in classes that seem to be part of external libraries.

@carakas carakas marked this pull request as draft September 20, 2023 20:50
tijsverkoyen
tijsverkoyen reviewed Sep 21, 2023
View reviewed changes
src/Themes/Fork/templates/Frontend/base/Head.html.twig Outdated
@@ -31,7 +31,7 @@
{# Google Fonts #}
<link href="https://fonts.googleapis.com/css?family=Courgette|Roboto:300,400,500,700,900" rel="stylesheet">

{% for css_file in css_files %}
{% for css_file in cssFiles %}
Copy link
Member

Choose a reason for hiding this comment

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

Dit snap ik niet? variabelen moeten toch met _ zijn? Lijkt me dat je het in PHP moet aanpassen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enkel als je ze via {% set = ...%} of in een loop definieert

Copy link
Member

Choose a reason for hiding this comment

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

@bjorvack I am following @tijsverkoyen in this as well; it had been bugging me throughout this review. I'm not too fond of the inconsistency, either all with _ or all camel case

@codecov
Copy link

codecov bot commented Sep 21, 2023 •
edited

Codecov Report

Attention: 114 lines in your changes are missing coverage. Please review.

Comparison is base (b1b0c92) 27.81% compared to head (e3c098f) 27.67%.

Files Patch % Lines
src/Core/Ruleset/TwigcsForkRuleset.php 0.00% 15 Missing ⚠️
...rm/Validator/UniqueDataTransferObjectValidator.php 0.00% 8 Missing ⚠️
...dules/Backend/Controller/BackendAjaxController.php 0.00% 6 Missing ⚠️
...c/Modules/Backend/Controller/BackendController.php 0.00% 6 Missing ⚠️
...Extensions/Backend/Actions/ThemeTemplateExport.php 0.00% 6 Missing ⚠️
.../Modules/Frontend/Domain/Block/BlockRepository.php 0.00% 6 Missing ⚠️
...ation/Domain/Translation/TranslationDomainType.php 0.00% 6 Missing ⚠️
src/Modules/Pages/Backend/Actions/PageAdd.php 0.00% 5 Missing ⚠️
...Pages/Domain/Revision/Form/RevisionContentType.php 0.00% 5 Missing ⚠️
src/Core/Domain/Header/Header.php 0.00% 4 Missing ⚠️
... and 18 more
Additional details and impacted files 
@@             Coverage Diff              @@
##              fork6    #3561      +/-   ##
============================================
- Coverage     27.81%   27.67%   -0.14%     
- Complexity     2629     2632       +3     
============================================
  Files           404      405       +1     
  Lines         10393    10485      +92     
============================================
+ Hits           2891     2902      +11     
- Misses         7502     7583      +81
Flag Coverage Δ
functional 0.00% <0.00%> (ø)
installer 27.62% <15.55%> (-0.14%) ⬇️
unit 0.05% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tijsverkoyen
Copy link
Member

Als deze PR klaar is lijkt het me wel verstanding om die te squashen want zijn heel veel zaken die gedaan worden en dan terug ongedaan en ...

@tijsverkoyen tijsverkoyen changed the title Fix code styles [invalid] Fix code styles Nov 17, 2023
@tijsverkoyen
Copy link
Member

This PR is not relevant anymore. #3578 is the successor.

@tijsverkoyen tijsverkoyen removed the request for review from daphneslootmans November 17, 2023 07:50
@bjorvack bjorvack closed this Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tijsverkoyen tijsverkoyen tijsverkoyen left review comments

@daphneslootmans daphneslootmans daphneslootmans left review comments

@carakas carakas carakas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bjorvack @tijsverkoyen @carakas @daphneslootmans