Skip to content
This repository has been archived by the owner on May 2, 2023. It is now read-only.

fix(tag): fix markup for removable tags - TWIG-78 #182

Merged
merged 3 commits into from
Oct 22, 2019
Merged

Conversation

libetho
Copy link
Contributor

@libetho libetho commented Oct 17, 2019

PR description

Please drop a few lines about the PR: what it does, how to test it, etc.

QA Checklist

In order to ensure a safe and quick review, please check that your PR follow those guidelines:

  • I have put the vanilla component as devDependencies
  • I have put the specs package as devDependencies
  • I have added the components directly used in the twig file (with include or embed) as dependencies
  • My component is listed in @ecl-twig/ec-components's dependencies
  • My variables naming follow the guidelines (snake case for twig)
  • I have provided tests
  • I have provided documentation (for the "notes" tab)
  • If my local yarn.lock contains changes, I have committed it
  • I have given my PR the proper label (pr: review needed to indicate that I'm done and now waiting for a review ,pr: wip to indicate that I'm actively working on it ...)

@libetho libetho added the pr: review needed Use this label to show that your PR needs to be review label Oct 17, 2019
@planctus planctus self-assigned this Oct 18, 2019
@planctus planctus added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Oct 18, 2019
Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

Merci @libetho, it's very much ok, just two small things that i found, one of that should now be discovered by our sniffers if we manage to fix their conf.. :)

@@ -37,6 +38,9 @@

{% if _tag.type == 'removable' %}
{% set _css_class = _css_class ~ ' ecl-tag--removable' %}
{% if _tag.aria_label is defined and _tag.aria_label is not empty %}
Copy link
Contributor

Choose a reason for hiding this comment

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

@libetho , here we have an extra space.
In the meantime we merged the fix for the configuration of our linters, if you managed to have php7 in your env you can install it following the docs (getting-started) and it should normally run on every commit checking your staged files. There also other scripts in the composer.json that you can use if you have the libraries installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra space has been removed

<a href="{{ _tag.path }}" class="{{ _css_class }}"{{ _extra_attributes|raw }}>
{{- _tag.label -}}
</a>
{% elseif _tag.type == 'button' %}
<button class="{{ _css_class }}" type="button"{{ _extra_attributes|raw }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

i know this might be inconsistent in all the templates, but why not starting fixing these inconsistencies? :) here i don't get the reason why we put a space between the classes and the type but not between the type and the extra_attributes. Both are correct, but it's weird to have this inconsistency, imho ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put spaces between all attributes. From point of view its more readable

@libetho libetho added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Oct 21, 2019
@planctus planctus added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Oct 22, 2019
@planctus planctus merged commit e2193be into develop Oct 22, 2019
@planctus planctus deleted the TWIG-78 branch October 22, 2019 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants