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

fix(button): Setting data-ecl-icon on the icon when used in a button - TWIG-199 #291

Merged
merged 14 commits into from
Jan 22, 2020

Conversation

Joosthe
Copy link
Contributor

@Joosthe Joosthe commented Jan 17, 2020

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 ...)

@Joosthe Joosthe added the pr: review needed Use this label to show that your PR needs to be review label Jan 17, 2020
@Joosthe Joosthe changed the title Twig 199 improve accordion fix(deprecated accordion): Improve the deprecated accordion component TWIG-199 Jan 17, 2020
@planctus planctus added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Jan 17, 2020
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.

Well, the diff is perfect, but please read the comments, especially the one about the inclusion of the button component. You can replay to comments if you feel you want to. ;)

</h{{ _item.level|default(3) }}>
<div
class="ecl-accordion__content"
hidden
id="{{ _item.id }}"
id="{{ _item.id }}-content"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure why this is done like this in ECL, might be that this a way to "target" the old accordion (this is the deprecated one).

@Joosthe Joosthe added the pr: review needed Use this label to show that your PR needs to be review label Jan 17, 2020
@planctus planctus changed the title fix(deprecated accordion): Improve the deprecated accordion component TWIG-199 fix(button): Setting data-ecl-icon on the icon when used in a button - TWIG-199 Jan 20, 2020
@planctus planctus added pr: modification needed and removed pr: review needed Use this label to show that your PR needs to be review labels Jan 20, 2020
@Joosthe Joosthe added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Jan 21, 2020
@planctus planctus removed the pr: review needed Use this label to show that your PR needs to be review label Jan 22, 2020
@planctus planctus merged commit 3e5aa22 into develop Jan 22, 2020
@planctus planctus deleted the twig-199-improve-accordion branch January 22, 2020 15:03
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