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

feat(description-list): horizontal variant - TWIG-39 #139

Merged
merged 13 commits into from
Sep 25, 2019
Merged

Conversation

planctus
Copy link
Contributor

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

@planctus planctus added the pr: review needed Use this label to show that your PR needs to be review label Sep 16, 2019
Copy link
Contributor

@papegaill papegaill left a comment

Choose a reason for hiding this comment

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

Just small things, otherwise it seems perfect 👍

@papegaill papegaill added pr: modification needed and removed pr: review needed Use this label to show that your PR needs to be review labels Sep 18, 2019
@planctus planctus added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Sep 18, 2019
@emeryro emeryro self-assigned this Sep 20, 2019
@emeryro emeryro added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Sep 20, 2019
Copy link
Contributor

@emeryro emeryro left a comment

Choose a reason for hiding this comment

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

some more small fixes needed here

@@ -47,6 +47,43 @@ exports[`EC - Description list Default renders correctly 1`] = `
</dl>
`;

exports[`EC - Description list Default renders correctly in the horizontal variant 1`] = `
<dl
class="ecl-description-list"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not the horizontal version tested here (missing css class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, true, i assumed that the variant was coming from the spec package without checking it...

src/ec/packages/ec-component-description-list/package.json Outdated Show resolved Hide resolved
@planctus planctus added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Sep 20, 2019
@emeryro
Copy link
Contributor

emeryro commented Sep 20, 2019

Looks good for me.
@papegaill could you validate too, as you requested changes?

@emeryro emeryro merged commit d6e269a into v2.11-dev Sep 25, 2019
@emeryro emeryro deleted the TWIG-39 branch September 25, 2019 12:33
@emeryro emeryro removed the pr: review needed Use this label to show that your PR needs to be review label Sep 25, 2019
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.

3 participants