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

refactor(demo): Pagebanner optional/mandatory knobs - FRONT-913 #374

Merged
merged 9 commits into from Mar 23, 2020

Conversation

Joosthe
Copy link
Contributor

@Joosthe Joosthe commented Mar 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 pr: review needed Use this label to show that your PR needs to be review tag: enhancement labels Mar 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.

It's the same review i wrote for the hero banner, basically, @Joosthe , it's better in my opinion if we focus on that and only after we move on this, ok?

@planctus planctus added pr: modification needed and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 17, 2020
data.title = text('Title', data.title, buttonLabels.required);
}

if (data.title) {
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate code

@Joosthe Joosthe added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Mar 18, 2020
@planctus
Copy link
Contributor

As i already tried the first time, @Joosthe , it is better if you focus on one of the pull requests, i'd say the hero banner since it's where i left the comments, and only after we get to a satisfactory state in that pr you will touch again this, otherwise it's a duplicated work on your side and on the review side, ok? ;)

@planctus planctus added pr: wip Mark the PR as WIP and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 19, 2020
@Joosthe Joosthe changed the title refactor(demo): Pagebanner prepared - FRONT-913 refactor(demo): Pagebanner optional/mandatory knobs - FRONT-913 Mar 19, 2020
@planctus planctus added pr: review needed Use this label to show that your PR needs to be review and removed pr: wip Mark the PR as WIP labels Mar 20, 2020
@papegaill papegaill added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 23, 2020
@papegaill papegaill merged commit bce35a8 into develop Mar 23, 2020
@papegaill papegaill deleted the FRONT-913 branch March 23, 2020 11:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants