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

fix(breadcrumb): component review - TWIG-205 #294

Merged
merged 12 commits into from
Jan 23, 2020
Merged

fix(breadcrumb): component review - TWIG-205 #294

merged 12 commits into from
Jan 23, 2020

Conversation

Joosthe
Copy link
Contributor

@Joosthe Joosthe commented Jan 22, 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 changed the base branch from master to develop January 22, 2020 09:40
@Joosthe Joosthe added the pr: review needed Use this label to show that your PR needs to be review label Jan 22, 2020
@planctus planctus changed the title fix(review): breadcrumb - TWIG-205 fix(breadcrumb): component review - TWIG-205 Jan 22, 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 22, 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.

@Joosthe, i think we talked about this, the idea in general is that if it's possible, we move what we might have been doing in a story file to pass some data to the component, to the specs (the adapter in this case), so that also when the component is rendered programmatically (js and php) we get the same rendering.
In this case for instance the diff is still showing differences that we can easily and properly fix by applying those information in the adapter instead of doing it in the story.
If there's still lack of clarity about this, please ask about any doubt you may have.

@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 23, 2020
Copy link
Contributor Author

@Joosthe Joosthe left a comment

Choose a reason for hiding this comment

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

Oke i have changed the values will now be loade from the specs

@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 23, 2020
@planctus
Copy link
Contributor

@Joosthe , i pushed a commit to make the diff entirely green, it also contains a simplification of the tests (it all comes from the specs right now), take a look and see if it's good for you ;)

@planctus planctus added the pr: review needed Use this label to show that your PR needs to be review label Jan 23, 2020
@planctus planctus removed the pr: review needed Use this label to show that your PR needs to be review label Jan 23, 2020
@planctus planctus merged commit b01610c into develop Jan 23, 2020
@planctus planctus deleted the TWIG-205 branch January 23, 2020 15:32
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