feat: EC Search Form #28

nimek2 · 2019-02-20T14:05:41Z

No description provided.

hapertown · 2019-02-20T19:14:43Z

@emeryro
Look at the tests (https://drone.fpfis.eu/ec-europa/ecl-twig/164/5) - I think this is the same problem as in here (#21). Lost dependency.
If we can solve this problem there, we will solve it and here the component will be ready for review.
Please for your help.

emeryro · 2019-02-21T06:59:14Z

The message in Drone says Your git status is not clean. Please update yarn.lock. Aborting.
Did you try to run yarn to update it?

hapertown · 2019-02-21T09:58:32Z

The message in Drone says Your git status is not clean. Please update yarn.lock. Aborting.
Did you try to run yarn to update it?

As you have seen many times, I updated the yarn and yarn.lock file but without success. I will analyze the changes to have knowledge for the future. You can now perform a component review.
Thanks for your help

emeryro

Visually the component is fine.
Just a few minor things to be updated

emeryro · 2019-02-21T10:10:18Z

src/ec/packages/ec-component-search-form/search-form.story.js

+import searchFormDocs from './docs/search-form.md';
+import searchForm from './search-form.html.twig';
+
+storiesOf('Components/SearchForm', module)


that's a small thing, but display in storybook will be better if you add a space between Search and Form here (that's how the item is displayed in the menu)

Fixed in 72483ba

emeryro · 2019-02-21T10:12:56Z

src/ec/packages/ec-component-search-form/docs/search-form.md

+### Example:
+
+```twig
+{% include 'path/to/icon.html.twig' with {


by adding two spaces at the end of the line, you should be able to force a new line. It would make the code more readable (currently it is a single very long line)

I would like to understand better what you mean - I understand that there is a lack of breaking the line, but what is the exact line(s) in this case?

I also dont get it for me it loo
ks ok https://github.com/ec-europa/ecl-twig/blob/ec-component-search-form/src/ec/packages/ec-component-search-form/docs/search-form.md

by adding two spaces at the end of the line, you should be able to force a new line. It would make the code more readable (currently it is a single very long line)

Ok I understand - you mean displaying in the Storybook (Notes section). Fixed in 0d2dc7f

emeryro · 2019-02-21T10:19:58Z

src/ec/packages/ec-component-search-form/docs/search-form.md

+
+### Parameters
+
+- "textInput" (associative array) default: A predefined structure for EC Text Input


as we are using underscores (snake case) for all other twig variable names, we should use it there too.
(note that it only for twig variables, javascript variables are still in camel case)

Fixed in 72483ba. Please check it.

emeryro · 2019-02-21T10:20:56Z

src/ec/packages/ec-component-search-form/package.json

+    "access": "public"
+  },
+  "devDependencies": {
+    "@ecl/ec-specs-search-form": "~2.1.0"


please add a devDependency to vanilla EC component too

Added in 72483ba

emeryro · 2019-02-21T10:22:52Z

src/ec/packages/ec-component-search-form/package.json

+  "description": "ECL Twig - EC Search Form",
+  "publishConfig": {
+    "access": "public"
+  },


as ec-component-text-input and ec-component-button are used directly in the template, they should be added to dependencies

Added in 72483ba

sorry, I meant a dependency to the twig component @ecl-twig/ec-component-text-input and @ecl-twig/ec-component-button
As they are included in the twig template, if they are not present it will crash. So the dependency is to prevent that.
Also these two should probably be in dependenciesinstead of devDependencies

emeryro · 2019-02-21T10:30:02Z

src/ec/packages/ec-component-search-form/search-form.html.twig

+    {% include '../ec-component-text-input/text-input.html.twig' with textInput %}
+  {% endif %}
+
+  {% if textInput is defined %}


should be button instead of textInput

Yes my mistake. Fixed in 72483ba

emeryro · 2019-02-21T10:34:14Z

src/ec/packages/ec-component-search-form/search-form.story.js

+        textInput: {
+          id: 'input-search',
+          name: 'search',
+          extra_classes: 'ecl-search-form__text-input',


there should be a (hidden) label for the text input. Even if it is not displayed to the users, it offers better accessibility (for screen reader for instance)

Please check if I added it well in 72483ba

You should add a label key in your text_input object:

text_input: { ... label: 'Search', ... }

emeryro

Almost there, please check the comments

emeryro · 2019-02-21T12:34:44Z

src/ec/packages/ec-component-search-form/search-form.story.js

+        textInput: {
+          id: 'input-search',
+          name: 'search',
+          extra_classes: 'ecl-search-form__text-input',


You should add a label key in your text_input object:

text_input: { ... label: 'Search', ... }

emeryro · 2019-02-21T12:38:42Z

src/ec/packages/ec-component-search-form/package.json

+  "description": "ECL Twig - EC Search Form",
+  "publishConfig": {
+    "access": "public"
+  },


sorry, I meant a dependency to the twig component @ecl-twig/ec-component-text-input and @ecl-twig/ec-component-button
As they are included in the twig template, if they are not present it will crash. So the dependency is to prevent that.
Also these two should probably be in dependenciesinstead of devDependencies

hapertown · 2019-02-21T12:52:42Z

When I try to add the dependencies given by you, I get an error:
Could not find package "@ ecl-twig / ec-component-text-input @ ^ 2.1.0" and I can not generate the correct yarn.lock file. Are you sure this is the correct package name?

The label added in 79ea3aa.

emeryro · 2019-02-21T13:35:44Z

the package name is good, but not the version.
Check in the package.json for these components:

{
  "name": "@ecl-twig/ec-component-text-input",
  "author": "European Commission",
  "license": "EUPL-1.1",
  "version": "0.0.1-alpha",
  ...
}

So that's perfectly normal if it doesn't find version 2.1.0 of this component

hapertown · 2019-02-21T13:50:57Z

the package name is good, but not the version.
Check in the package.json for these components:
{
  "name": "@ecl-twig/ec-component-text-input",
  "author": "European Commission",
  "license": "EUPL-1.1",
  "version": "0.0.1-alpha",
  ...
}
So that's perfectly normal if it doesn't find version 2.1.0 of this component

Ok, check if now will be ok 4edf092

Matt added 2 commits February 20, 2019 14:33

Initial commit for Search Form

bce05d3

Merge branch 'master' into ec-component-search-form

46a5e3d

Matt and others added 4 commits February 21, 2019 08:24

Update yarn.lock

5263bec

Update yarn.lock v2

6739b46

Update yarn.lock v3

57460e9

update yarn

e0895d6

emeryro self-assigned this Feb 21, 2019

emeryro added the pr: under review label Feb 21, 2019

emeryro suggested changes Feb 21, 2019

View reviewed changes

emeryro added pr: modification needed and removed pr: under review labels Feb 21, 2019

emeryro removed their assignment Feb 21, 2019

Fix bugs

72483ba

nimek2 added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Feb 21, 2019

Update search-form.md (Fix code display)

0d2dc7f

emeryro added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Feb 21, 2019

emeryro self-assigned this Feb 21, 2019

emeryro suggested changes Feb 21, 2019

View reviewed changes

emeryro added pr: modification needed and removed pr: under review labels Feb 21, 2019

emeryro removed their assignment Feb 21, 2019

Add Label knob for text_input

79ea3aa

hapertown mentioned this pull request Feb 21, 2019

feat: Ec component card #21

Merged

Update yarn.lock and package.json dependencies

4edf092

emeryro added pr: under review and removed pr: modification needed labels Feb 21, 2019

emeryro self-assigned this Feb 21, 2019

emeryro approved these changes Feb 21, 2019

View reviewed changes

emeryro removed the pr: under review label Feb 21, 2019

emeryro merged commit e6ebcca into master Feb 21, 2019

emeryro deleted the ec-component-search-form branch February 21, 2019 13:58

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

yhuard unassigned emeryro Feb 27, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: EC Search Form #28

feat: EC Search Form #28

nimek2 commented Feb 20, 2019

hapertown commented Feb 20, 2019 •

edited

Loading

emeryro commented Feb 21, 2019

hapertown commented Feb 21, 2019 •

edited

Loading

emeryro left a comment

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

nimek2 Feb 21, 2019 •

edited

Loading

hapertown Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

hapertown Feb 21, 2019

emeryro Feb 21, 2019

emeryro left a comment

emeryro Feb 21, 2019

emeryro Feb 21, 2019

hapertown commented Feb 21, 2019 •

edited

Loading

emeryro commented Feb 21, 2019 •

edited

Loading

hapertown commented Feb 21, 2019


		### Parameters

		- "textInput" (associative array) default: A predefined structure for EC Text Input

feat: EC Search Form #28

feat: EC Search Form #28

Conversation

nimek2 commented Feb 20, 2019

hapertown commented Feb 20, 2019 • edited Loading

emeryro commented Feb 21, 2019

hapertown commented Feb 21, 2019 • edited Loading

emeryro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimek2 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emeryro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hapertown commented Feb 21, 2019 • edited Loading

emeryro commented Feb 21, 2019 • edited Loading

hapertown commented Feb 21, 2019

hapertown commented Feb 20, 2019 •

edited

Loading

hapertown commented Feb 21, 2019 •

edited

Loading

nimek2 Feb 21, 2019 •

edited

Loading

hapertown commented Feb 21, 2019 •

edited

Loading

emeryro commented Feb 21, 2019 •

edited

Loading