feat: implement timeline2 component - INNO-1546 #102

haringsrob · 2019-06-21T10:48:57Z

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

yhuard

Impressive work so far 👍 I've spotted some small things to enhance or fix here and there, nothing dramatic.

Regarding the title of the PR, we try to follow Angular's conventions with a small change: we add the Jira ticket at the end of the title. Could you please update it? 😉

src/ec/packages/ec-component-timeline/demo/data.js

src/ec/packages/ec-component-timeline/timeline.html.twig

haringsrob · 2019-06-24T10:12:18Z

Hi @yhuard,

In addition to your comments some other fixes were made as well.

Leftover "accordion" in the docs.
A leftover
in the twig file.

The button is now using the component and I added the extra icon classes parameter.

I removed the button attribute from the component paremeters, not sure where I got that from..

Other remarks were due to copy pasting. Adjusted those.

…ecl-twig into INNO-1546-timeline2-component

src/ec/packages/ec-component-timeline/docs/timeline.md

src/ec/packages/ec-component-timeline/timeline.html.twig

src/ec/packages/ec-component-timeline/timeline.test.js

src/ec/packages/ec-component-timeline/timeline.story.js

haringsrob · 2019-06-27T11:53:03Z

Hi @yhuard I have taken care of the comments.

yhuard

Technically, it works well! Now there are just some small cosmetic changes that you could make and then we can merge this PR 🎉

yhuard · 2019-06-28T08:07:48Z

src/ec/packages/ec-component-timeline/demo/data.js

+  }
+
+  return {
+    toggle_collapsed: 'Show %d more items'.replace('%d', hiddenCount),


Suggested change

toggle_collapsed: 'Show %d more items'.replace('%d', hiddenCount),

toggle_collapsed: `Show ${hiddenCount} more items`,

yhuard · 2019-06-28T08:08:22Z

src/ec/packages/ec-component-timeline/demo/data.js

+
+  return {
+    toggle_collapsed: 'Show %d more items'.replace('%d', hiddenCount),
+    toggle_expanded: 'Hide %d items'.replace('%d', hiddenCount),


Suggested change

toggle_expanded: 'Hide %d items'.replace('%d', hiddenCount),

toggle_expanded: `Hide ${hiddenCount} items`,

yhuard · 2019-06-28T08:12:37Z

src/ec/packages/ec-component-timeline/demo/data.js

+  if (to > 0) {
+    hiddenCount = to - from;
+  } else {
+    hiddenCount = specData.items.length - (from + Math.abs(to));


or simpler:

Suggested change

hiddenCount = specData.items.length - (from + Math.abs(to));

hiddenCount = initialData.items.length + to - from;

yhuard · 2019-06-28T08:16:16Z

src/ec/packages/ec-component-timeline/timeline.story.js

+      if (to > 0) {
+        hiddenCount = to - from;
+      } else {
+        hiddenCount = demoData.items.length - (from + Math.abs(to));


same remark as earlier, it could be simplified a bit :)

yhuard · 2019-06-28T08:17:36Z

src/ec/packages/ec-component-timeline/timeline.story.js

+  .add(
+    'default',
+    () => {
+      const from = number('From', 10);


what about using demoData.hide.from as a default instead of 10? (same for to)
thus the changes made to demo/data.js would be reflected here as well

yhuard · 2019-06-28T08:19:07Z

src/ec/packages/ec-component-timeline/timeline.story.js

+
+      const fullDemoData = {
+        ...demoData,
+        toggle_collapsed: 'Show %d more items'.replace('%d', hiddenCount),


same remark as earlier, template literals are preferable (more JS-y)

src/ec/packages/ec-component-timeline/timeline.html.twig

…46-timeline2-component

yhuard

Great work! 🎉

haringsrob added 3 commits June 21, 2019 12:42

INNO-1546 Timeline component without JS

e577e8a

INNO-1546 Test updates

9ce12d9

INNO-1546 add missing dependency in ec-components

213814a

haringsrob added the pr: wip Mark the PR as WIP label Jun 21, 2019

haringsrob changed the title ~~Inno 1546 timeline2 component~~ wip: INNO-1546 timeline2 component Jun 21, 2019

haringsrob changed the base branch from master to v2.7-dev June 21, 2019 10:49

INNO-1546 update tests and add js

67067d6

haringsrob 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 Jun 21, 2019

haringsrob changed the title ~~wip: INNO-1546 timeline2 component~~ feat: INNO-1546 timeline2 component Jun 21, 2019

Merge branch 'v2.7-dev' into INNO-1546-timeline2-component

0f67d36

yhuard self-assigned this Jun 24, 2019

yhuard added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Jun 24, 2019

yhuard suggested changes Jun 24, 2019

View reviewed changes

src/ec/packages/ec-component-timeline/demo/data.js Outdated Show resolved Hide resolved

src/ec/packages/ec-component-timeline/timeline.html.twig Outdated Show resolved Hide resolved

src/ec/packages/ec-component-timeline/timeline.html.twig Outdated Show resolved Hide resolved

yhuard removed their assignment Jun 24, 2019

yhuard added pr: modification needed and removed pr: under review labels Jun 24, 2019

yhuard reviewed Jun 24, 2019

View reviewed changes

src/ec/packages/ec-component-timeline/timeline.html.twig Outdated Show resolved Hide resolved

haringsrob added pr: wip Mark the PR as WIP and removed pr: modification needed labels Jun 24, 2019

haringsrob changed the title ~~feat: INNO-1546 timeline2 component~~ feat: timeline2 component - INNO-1546 Jun 24, 2019

haringsrob added 2 commits June 24, 2019 12:08

INNO-1546 bugfixes and cleanup

0e05003

INNO-1546 update docs

c96e7fb

haringsrob 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 Jun 24, 2019

haringsrob added 3 commits June 24, 2019 12:13

Merge branch 'v2.7-dev' into INNO-1546-timeline2-component

81b6f7f

INNO-1546 update button component and documentation

54c2bcc

Merge branch 'INNO-1546-timeline2-component' of github.com:ec-europa/…

e139453

…ecl-twig into INNO-1546-timeline2-component

haringsrob added the pr: review needed Use this label to show that your PR needs to be review label Jun 27, 2019

yhuard added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Jun 27, 2019

yhuard self-assigned this Jun 27, 2019

yhuard suggested changes Jun 27, 2019

View reviewed changes

yhuard added pr: modification needed and removed pr: under review labels Jun 27, 2019

yhuard removed their assignment Jun 27, 2019

INNO-1546 add logic for positive to, tests improvements, knobs

bb6ac5f

haringsrob added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Jun 27, 2019

yhuard added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Jun 28, 2019

yhuard self-assigned this Jun 28, 2019

yhuard suggested changes Jun 28, 2019

View reviewed changes

yhuard removed the pr: under review label Jun 28, 2019

yhuard removed their assignment Jun 28, 2019

yhuard added the pr: modification needed label Jun 28, 2019

INNO-1546: Improve code readability

2c783e6

haringsrob added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Jun 28, 2019

haringsrob added 2 commits June 28, 2019 15:50

Merge branch 'v2.7-dev' of github.com:ec-europa/ecl-twig into INNO-15…

5b85c89

…46-timeline2-component

INNO-1546 give the comment some space

f214363

yhuard added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Jun 28, 2019

yhuard approved these changes Jun 28, 2019

View reviewed changes

yhuard removed the pr: under review label Jun 28, 2019

yhuard merged commit 1406b9d into v2.7-dev Jun 28, 2019

yhuard deleted the INNO-1546-timeline2-component branch June 28, 2019 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement timeline2 component - INNO-1546 #102

feat: implement timeline2 component - INNO-1546 #102

haringsrob commented Jun 21, 2019

yhuard left a comment

haringsrob commented Jun 24, 2019

haringsrob commented Jun 27, 2019

yhuard left a comment

yhuard Jun 28, 2019

yhuard Jun 28, 2019

yhuard Jun 28, 2019

yhuard Jun 28, 2019

yhuard Jun 28, 2019

yhuard Jun 28, 2019

yhuard left a comment

	toggle_collapsed: 'Show %d more items'.replace('%d', hiddenCount),
	toggle_collapsed: `Show ${hiddenCount} more items`,

	toggle_expanded: 'Hide %d items'.replace('%d', hiddenCount),
	toggle_expanded: `Hide ${hiddenCount} items`,

	hiddenCount = specData.items.length - (from + Math.abs(to));
	hiddenCount = initialData.items.length + to - from;

feat: implement timeline2 component - INNO-1546 #102

feat: implement timeline2 component - INNO-1546 #102

Conversation

haringsrob commented Jun 21, 2019

PR description

QA Checklist

yhuard left a comment

Choose a reason for hiding this comment

haringsrob commented Jun 24, 2019

haringsrob commented Jun 27, 2019

yhuard left a comment

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard Jun 28, 2019

Choose a reason for hiding this comment

yhuard left a comment

Choose a reason for hiding this comment