feat: Ec component site header #33

nimek2 · 2019-02-22T09:39:26Z

No description provided.

emeryro · 2019-02-22T09:50:05Z

Before going any further in the PR, I have to ask: why is there a version of EU link here?
It may conflict with the PR dedicated to EU links, and as far as I know, it is not required for EC site header.

hapertown · 2019-02-22T09:51:14Z

@emeryro
Look at the Header image src - path is incorrect (no image). Let me know if you want to set the path to the graphic statically (then I ask for the correct URL) or download it from some resource (just like defaultSprite).

Question # 2 - in this component there are elements of EC Link. We could use this component, the problem is only that the EC Link component supports plain text, not all HTML.
Is the current solution enough for you at this stage whether EC Link must support HTML as a label parameter? Then we would have to do it as a block in TWIG.

hapertown · 2019-02-22T10:04:04Z

Before going any further in the PR, I have to ask: why is there a version of EU link here?
It may conflict with the PR dedicated to EU links, and as far as I know, it is not required for EC site header.

This is my mistake - I made a new branch in the wrong way. Everything fixed in 9db993d. I'll just add the correct yarn.lock file.

hapertown · 2019-02-22T10:11:42Z

@emeryro Already everything is ok, you can do a review.

emeryro · 2019-02-22T13:32:51Z

src/ec/packages/ec-component-site-header/docs/site-header.md

+- "search_form" (associative array) (default: predefined structure): EC Search Form component structure
+- "language" (associative array) (default: predefined structure): Language switcher settings. Default settings for English. format:
+  - "url": (string) (default: ''): URL for switcher
+  - "label": (string) (default: 'English'): Switcher language label, eg. 'English', 'Français', etc.


We should not enforce default language (politcal reasons...). So instead of using english as default, fields should be empty (in every files)

emeryro · 2019-02-22T13:41:55Z

src/ec/packages/ec-component-site-header/site-header.story.js

+          aria_label: 'Commmission Européenne',
+        },
+        header_image: {
+          src: 'static/media/logo--fr.a8aaa7ab.svg',


you can find the logos in package @ecl/ec-resources-logo: https://github.com/ec-europa/europa-component-library/tree/v2-dev/src/systems/ec/resources/logo

you can find the logos in package @ecl/ec-resources-logo: https://github.com/ec-europa/europa-component-library/tree/v2-dev/src/systems/ec/resources/logo

I have already made changes locally but I need help.

Could you tell me how to correctly configure the webpack.config.js file so that svg files are not compiled to one icon.svg but separately - so that I can implement them in the Site Header.

Now, when I build the EC environment, I have one svg file (icon.svg), the content of which is equal to the last imported svg, eg from such a resource: import headerFrenchLogo from @ecl/ec-resources-logo/logo-fr.svg.

Thank you for your help.

it will be fixed in my PR on language list, but you can already apply the changes if you want: https://github.com/ec-europa/ecl-twig/pull/27/files#diff-ed535e5276105399f01342171198a43eR19

Changed in 7fac7df

hapertown · 2019-02-25T12:57:18Z

@emeryro
I fixed all files according to your instructions, see please.
Unfortunately, I have problems with tests - locally tests are ok, buto after commit, tests have problems with breaklines (look at current tests).
This problem occurs practically every time. Updating npm and calling yarn do not help. Can we somehow synchronize my local environment with the test environment?

emeryro · 2019-02-26T07:32:18Z

I tried to run tests locally on your branch, and I got the same error as Drone reported (running yarn, yarn dist, yarn jest).
What environement are you developping on? Mac? Linux?
I am using Ubuntu with following versions

Node v8.12.0
yarn v1.13.0

hapertown · 2019-02-26T07:51:59Z

I tried to run tests locally on your branch, and I got the same error as Drone reported (running yarn, yarn dist, yarn jest).
What environement are you developping on? Mac? Linux?
I am using Ubuntu with following versions

Node v8.12.0

yarn v1.13.0

My versions are:
Ubuntu 16.04
NodeJS 11.10.0
Yarn 1.12.3

emeryro · 2019-02-27T07:06:45Z

src/ec/packages/ec-component-site-header/site-header.story.js

+        },
+        search_form: {
+          button: {
+            label: 'Search',


Please add icon information here too (for english and translated). Currently the icon isn't displayed on mobile version.

Fixed in 659a80d

emeryro · 2019-02-27T07:09:08Z

src/ec/packages/ec-component-site-header/site-header.html.twig

+        </a>
+      </div>
+    </div>
+    {%- include '../ec-component-search-form/search-form.html.twig' with _search_form -%}


missing class ecl-site-header__search here

Fixed in 659a80d

I had to modify the file search-form.html.twig in EC Search Form in c89d15d because I founded a small bug there. But it does not affect the tests or other files so I think we can approve it here.

Matt added 4 commits February 21, 2019 16:35

Initial commit for EU Link

9dd8553

Update README.md

d06ee6a

Initial commit for EC Site Header

6a21348

Update tests

e325c0e

nimek2 added the pr: review needed Use this label to show that your PR needs to be review label Feb 22, 2019

Remove EU Link

9db993d

Update yarn.lock

d1734bc

Matt added 2 commits February 22, 2019 11:32

Resolve conflicts

edbb724

Update ec-components package.json

d60e9db

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

emeryro self-assigned this Feb 22, 2019

emeryro suggested changes Feb 22, 2019

View reviewed changes

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

emeryro removed their assignment Feb 22, 2019

Matt added 7 commits February 22, 2019 14:51

Merge branch 'master' into ec-component-site-header

93f4681

Remove default English settings

7616161

Merge branch 'master' into ec-component-site-header

e140dca

Update webpack.config.js for ec storybook

7fac7df

Update yarn.lock

55639db

Update tests

53542ab

Update tests v2

8bb5340

degliwe previously approved these changes Feb 26, 2019

View reviewed changes

Merge branch 'master' into ec-component-site-header

2bfa415

emeryro suggested changes Feb 27, 2019

View reviewed changes

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

emeryro unassigned degliwe Feb 27, 2019

Update EC Search Form twig file

c89d15d

nimek2 dismissed degliwe’s stale review via c89d15d February 27, 2019 07:31

Matt added 2 commits February 27, 2019 08:32

Update EC Site Header files

659a80d

Update tests

447cebc

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

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

emeryro self-assigned this Feb 27, 2019

emeryro approved these changes Feb 27, 2019

View reviewed changes

emeryro merged commit b06b04d into master Feb 27, 2019

emeryro deleted the ec-component-site-header branch February 27, 2019 07:52

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

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 component site header #33

feat: Ec component site header #33

nimek2 commented Feb 22, 2019

emeryro commented Feb 22, 2019

hapertown commented Feb 22, 2019

hapertown commented Feb 22, 2019 •

edited

Loading

hapertown commented Feb 22, 2019

emeryro Feb 22, 2019

emeryro Feb 22, 2019

hapertown Feb 22, 2019

emeryro Feb 25, 2019

hapertown Feb 25, 2019

hapertown commented Feb 25, 2019

emeryro commented Feb 26, 2019

hapertown commented Feb 26, 2019

emeryro Feb 27, 2019

hapertown Feb 27, 2019

emeryro Feb 27, 2019

hapertown Feb 27, 2019

feat: Ec component site header #33

feat: Ec component site header #33

Conversation

nimek2 commented Feb 22, 2019

emeryro commented Feb 22, 2019

hapertown commented Feb 22, 2019

hapertown commented Feb 22, 2019 • edited Loading

hapertown commented Feb 22, 2019

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

hapertown commented Feb 25, 2019

emeryro commented Feb 26, 2019

hapertown commented Feb 26, 2019

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

hapertown commented Feb 22, 2019 •

edited

Loading