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

feat: Ec component breadcrumb #37

Merged
merged 13 commits into from
Mar 1, 2019
Merged

feat: Ec component breadcrumb #37

merged 13 commits into from
Mar 1, 2019

Conversation

nimek2
Copy link
Contributor

@nimek2 nimek2 commented Feb 27, 2019 •
edited
Loading

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

@nimek2 nimek2 added the pr: review needed Use this label to show that your PR needs to be review label Feb 27, 2019
@emeryro 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 emeryro self-assigned this Feb 27, 2019
@emeryro
Copy link
Contributor

emeryro commented Feb 27, 2019

Before going any further, it seemsthat the javascript behavior is not implemented here.
On ECL version, there is an aditional script loaded for the breadcrumb to automatically display or hide elements depending on the available space.

We should have the same beahvior here.
The script in the the vanilla breadcrumbpackage: https://github.com/ec-europa/europa-component-library/tree/v2/src/systems/ec/implementations/vanilla/packages/ec-component-breadcrumb

@emeryro emeryro removed their assignment Feb 27, 2019
@hapertown
Copy link

@emeryro
Could you tell me how to properly load this storybook script?

@yhuard
Copy link
Contributor

yhuard commented Feb 27, 2019

@hapertown I'll come back to you with a solution for that ;)

@yhuard yhuard self-assigned this Feb 27, 2019
@yhuard yhuard removed their assignment Feb 28, 2019
@yhuard
Copy link
Contributor

yhuard commented Feb 28, 2019

@hapertown I've added the JS behavior. It's not as straightforward as I hoped but at least it's working (see https://github.com/ec-europa/ecl-twig/pull/37/files#diff-b2d39af2fbd791ba86ecf4cb6a737e0fR81) You don't have to do anything regarding the JS behaviour for the component, it's done. In the future, I might come back with a better solution.

@emeryro I guess you can review this PR now

@yhuard yhuard added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Feb 28, 2019
@kalinchernev kalinchernev self-assigned this Feb 28, 2019
@kalinchernev kalinchernev removed their assignment Feb 28, 2019
@kalinchernev
Copy link
Contributor

kalinchernev commented Feb 28, 2019 •
edited
Loading

Dear all, the javascript behavior works well now. I quickly checked the other basics and didn't find any issues.

@emeryro emeryro added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Feb 28, 2019
@emeryro emeryro self-assigned this Feb 28, 2019
emeryro
emeryro suggested changes Feb 28, 2019
View reviewed changes
src/ec/packages/ec-component-breadcrumb/breadcrumb.story.js Outdated
'simple',
() =>
breadcrumb({
links: [
Copy link
Contributor

Choose a reason for hiding this comment

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

When possible, please use the data from the spec component. That way, if we change data on ECL for whatever reason it will be updated everywhere.

You will probably have to map the structure of ECL data with yours, as it has been done here or here for instance.

Copy link

Choose a reason for hiding this comment

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

Fixed in cbbfd67

@emeryro emeryro removed their assignment Feb 28, 2019
@nimek2 nimek2 added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Mar 1, 2019
@hapertown
Copy link

@emeryro
I got this information: "@david (WAAT) I merged the pull request from Yannick about the DOM-focused tests".
Should we change something here in the breadcrumb.test.js, according to the changes that have been confirmed here?

@emeryro
Copy link
Contributor

emeryro commented Mar 1, 2019

Yes, please update it here.
I'm doing it for all other components

@hapertown
Copy link

Yes, please update it here.
I'm doing it for all other components

Updated in 92da83b

@emeryro emeryro merged commit 8d4a157 into master Mar 1, 2019
@emeryro emeryro removed the pr: review needed Use this label to show that your PR needs to be review label Mar 1, 2019
@emeryro emeryro deleted the ec-component-breadcrumb branch March 1, 2019 12:06
@yhuard yhuard removed pr: modification needed pr: review needed Use this label to show that your PR needs to be review pr: under review labels Mar 4, 2019
@hapertown hapertown mentioned this pull request Mar 7, 2019
Merged
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hapertown hapertown hapertown left review comments

@emeryro emeryro emeryro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nimek2 @emeryro @hapertown @yhuard @kalinchernev