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

Removed duplicated classes in form_fields.html.twig #853

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

todomagichere
Copy link
Contributor

@todomagichere todomagichere commented Jul 31, 2023

Question Answer
Tickets (or N/A)
Bug fix? no
New feature? no
BC breaks? no
Tests pass? no
Doc needed? no
License GPL-2.0

In form_fields.html.twig there are some classes duplicated. Is it necessary to have duplicity in these classes?

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

I think we could target 4.5 branch with this.

@ViniTou ViniTou requested review from a team August 2, 2023 06:20
@konradoboza konradoboza requested a review from a team August 2, 2023 06:22
{% if errors|length > 0 %}{% set wrapper_class = (wrapper_class|default('') ~ ' is-invalid')|trim %}{% endif %}

{% set label_attr = label_attr|merge({'class': (label_attr.class|default('') ~ ' ibexa-label ibexa-data-source__label ibexa-data-source__label')|trim}) %}
{% set label_wrapper_attr = label_wrapper_attr|default({})|merge({'class': (label_wrapper_attr.class|default('') ~ ' ibexa-data-source__label-wrapper ibexa-data-source__label-wrapper')|trim}) %}
{% set label_attr = label_attr|merge({'class': (label_attr.class|default('') ~ ' ibexa-label ibexa-data-source__label')|trim}) %}
Copy link
Contributor

@tischsoic tischsoic Aug 22, 2023

Choose a reason for hiding this comment

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

I'm not sure whether we should delete these duplicates. When we try to trace back where they come from we can see that first they were duplicated to have both ibexa- and ez-.

  1. c5cb76b#diff-3776f7fe593a6a85de0c62980e813efe1256ea7398646bff8c34a16ad83d91aaR45
  2. and then ez- was changed to ibexa- in this PR probably by a mistake:
    a4bdc57#diff-3776f7fe593a6a85de0c62980e813efe1256ea7398646bff8c34a16ad83d91aaR47

So probably we should revert duplicates to ez- to revert changes from 2nd PR? But maybe we no longer want to have ez-, especially given we have already version 4.5?

Suggested change
{% set label_attr = label_attr|merge({'class': (label_attr.class|default('') ~ ' ibexa-label ibexa-data-source__label')|trim}) %}
{% if required %}{% set wrapper_class = (wrapper_class ~ ' ibexa-data-source__field--required ez-data-source__field--required')|trim %}{% endif %}

Copy link
Member

Choose a reason for hiding this comment

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

As main branch currently points to 5.0 we can safely forget about ez- counterpart. @tischsoic Could you please take over this PR ?

@mikadamczyk
Copy link
Contributor

mikadamczyk commented Jul 9, 2024

@tischsoic Will you take over this PR? Also, rabese is needed here

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

Successfully merging this pull request may close these issues.