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

feat: EC component Tag #17

Merged
merged 8 commits into from
Feb 13, 2019
Merged

feat: EC component Tag #17

merged 8 commits into from
Feb 13, 2019

Conversation

nimek2
Copy link
Contributor

@nimek2 nimek2 commented Feb 8, 2019

Please nogte that this is deoendent on CC icon so have to be merger after that

@planctus planctus self-assigned this Feb 8, 2019
planctus
planctus suggested changes Feb 8, 2019
View reviewed changes
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.

Ec icon is already merged, but we have a failure in the test, here, would you fix this before we go into a full review of the component? Thanks a lot.

@hapertown
Copy link

For me, locally tests come out ok - later I will check what may be the reason for the wrong tests.

@hapertown
Copy link

@planctus the tests have been fixed in the last commit - you can review component now.

@emeryro emeryro assigned emeryro and unassigned planctus Feb 12, 2019
emeryro
emeryro suggested changes Feb 12, 2019
View reviewed changes
src/ec/packages/ec-component-tag/docs/tag.md Outdated
- "type" (string) (default: '') - type of tag (can be 'link', 'button', 'removable')
- "label" (string) (default: '') - tag label
- "default_icon_path" (string ) (default: '') - path for the icon image (need to render Icon component if tag is removable)
- "extra_classes" (optional) (string) (default: '') Extra classes (space separated) for the icon
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: this is tag and not icon here (and next line too)

Copy link

Choose a reason for hiding this comment

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

I don't know what you mean. The variable default_icon_path defines here the path to the file with icons, which is required for the tag type removable. Do you mean to change the variable name and description or define this path in the structure of the tag object. e.g.
tag: { type: 'removable', icon_path: '/pathtoicon' }?

Copy link
Contributor

Choose a reason for hiding this comment

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

no it's just the comment for extra_classes and extra_attributes. It should be tag instead of icon (but I agree that it is minor)

Copy link

Choose a reason for hiding this comment

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

Ok I see my mistake, I will correct everything after merging with the EC Icon.

Copy link

Choose a reason for hiding this comment

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

Fixed in 069ce05

src/ec/packages/ec-component-tag/tag.html.twig
{{ _tag.label }}
{% if _tag.type=='removable' %}
<span class="ecl-tag__icon">
{% include '../ec-component-icon/icon.html.twig' with { icon: { type: 'ui', name: 'close', size: 'xs', path: default_icon_path }, extra_classes: 'ecl-tag__icon-close' } %}
Copy link
Contributor

Choose a reason for hiding this comment

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

We recently added the extra attribute focusable="false" to all svg icons, to prevent a bug on Internet Explorer. Could you add it on your side too?
See https://v2--europa-component-library.netlify.com/ec/components/icon/code/

Copy link

Choose a reason for hiding this comment

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

Fixed in 069ce05

@emeryro emeryro removed their assignment Feb 12, 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 Feb 13, 2019
@nimek2
Copy link
Contributor Author

nimek2 commented Feb 13, 2019

All fixed here and test passed please make final review

@emeryro
Copy link
Contributor

emeryro commented Feb 13, 2019

Component seems good.
Could you just add a devDependency to the "spec" component, theway it has been done here?
In this case it would be "@ecl/ec-specs-tag": "~2.1.0"
We consider it as a good practice, to keep the component close to its specification.
Thanks

@hapertown
Copy link

hapertown commented Feb 13, 2019 •
edited
Loading

Component seems good.
Could you just add a devDependency to the "spec" component, theway it has been done here?
In this case it would be "@ecl/ec-specs-tag": "~2.1.0"
We consider it as a good practice, to keep the component close to its specification.
Thanks

Fixed in latest commit. Should I always add to every component @ecl/ec[eu]-specs-[component] in package.json?

@emeryro
Copy link
Contributor

emeryro commented Feb 13, 2019

Yes, please do so for every components

@planctus planctus self-assigned this Feb 13, 2019
@planctus planctus merged commit 05eb43d into master Feb 13, 2019
@degliwe degliwe deleted the ec-component-tag branch February 18, 2019 13:40
@yhuard yhuard removed pr: review needed Use this label to show that your PR needs to be review labels Feb 21, 2019
@yhuard yhuard removed the pr: review needed Use this label to show that your PR needs to be review label Feb 21, 2019
@yhuard yhuard removed pr: review needed Use this label to show that your PR needs to be review pr: under review pr: modification needed labels Feb 27, 2019
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

@planctus planctus planctus 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 @hapertown @emeryro @planctus @yhuard