Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(social-icons): upgrade component - INNO-693 #282

Merged
merged 12 commits into from
Jul 17, 2017
Merged

feat(social-icons): upgrade component - INNO-693 #282

merged 12 commits into from
Jul 17, 2017

Conversation

kalinchernev
Copy link
Contributor

@kalinchernev kalinchernev commented Jul 14, 2017 •
edited
Loading

PR description

  • I have added @ec-europa/ecl-typography-lists as a dependency, not the base, as that's what I need - is it wrong?
  • For the tests, I included the overall demo, not per-icon-and-per-modifier as it will be crazy. I decided to skip a11y tests for this reason, but tell me if that is a wrong step.
  • Also, I've left fixed sizing on icons, because I think it's necessary?
  • PNGs are removed
  • I've tried to make the template dynamic enough to handle several scenarios

QA Checklist

In order to ensure a safe and quick review, please check that your PR follow those guidelines:

  • package.json is up-to-date and @ec-europa/ecl-base is part of the dependencies
  • I have given the fractal status “ready” to my component
  • I have declared @define mycomponent in the SCSS file
  • I have specified margin: 0; on the CSS component
  • I have provided tests
  • I follow the naming guidelines
  • the component supports composition
  • the template is fully functional
  • I have filled the README.md file (at least a few lines)
  • I have checked the dependencies
  • there are no hardcoded strings (all content come from the context)

@kalinchernev kalinchernev requested a review from yhuard July 14, 2017 13:13
@kalinchernev kalinchernev changed the title Feat/social icons inno 693 feat(social-icons): upgrade component - INNO-693 Jul 14, 2017
@ec-europa ec-europa deleted a comment Jul 14, 2017
@ec-europa ec-europa deleted a comment Jul 14, 2017
@ec-europa ec-europa deleted a comment Jul 14, 2017
yhuard
yhuard suggested changes Jul 14, 2017
View reviewed changes
Copy link
Contributor

@yhuard yhuard left a comment

Choose a reason for hiding this comment

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

Few things to change or to explain ;)

framework/components/ecl-social-icons/package.json Outdated
"license": "EUPL-1.1"
"license": "EUPL-1.1",
"dependencies": {
"@ec-europa/ecl-typography-lists": "^0.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing base dependency

framework/components/ecl-social-icons/ecl-social-icons.scss
}
}

.ecl-icon-social {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have used flexboxes:

.ecl-icon-social {
  display: flex;
  align-items: center;
  margin: 0;
}

.ecl-icon-social::before {
  content: '';
  display: block;
  height: 2rem;
  margin-right: 1rem;
  width: 2rem;
}

And that's mostly it

framework/components/ecl-social-icons/ecl-social-icons.scss Outdated
}
}

.ecl-icon-social--smaller {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the nesting here. It's not very BEM-compliant :/

framework/components/ecl-social-icons/ecl-social-icons.twig Outdated

{# If there is a list, make a list #}
{% if list is defined and list is iterable %}
<ul class="{{ listExtraClasses ?: 'ecl-list' }}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about that. I thought the component was just about adding a social icon, not a whole list of links ^^'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd gladly remove, was an idea for making it simple to have a list with 1 include, rather than an include per item. I'll remove :)

@kalinchernev
Copy link
Contributor Author

Couldn't run the tests to update the refs:

 ~/projects/inno/europa-component-library   feat/social-icons-inno-693 ●✚  yarn test:functional -- --spec framework/components/ecl-social-icons/test/spec/social-icons.js
yarn test:functional v0.27.5
$ wdio test/wdio.conf.js "--spec" "framework/components/ecl-social-icons/test/spec/social-icons.js"
Visual regression tests will be run on all packages
Failed loading configuration file: /home/kalin/projects/inno/europa-component-library/test/wdio.conf.js
/home/kalin/projects/inno/europa-component-library/node_modules/webdriverio/build/lib/utils/ConfigParser.js:183
                throw e;
                ^

TypeError: Object.values is not a function
    at module.exports.getCapabilities (/home/kalin/projects/inno/europa-component-library/test/utils/capabilities.js:48:17)
    at Object.<anonymous> (/home/kalin/projects/inno/europa-component-library/test/wdio.conf.js:59:17)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at ConfigParser.addConfigFile (/home/kalin/projects/inno/europa-component-library/node_modules/webdriverio/build/lib/utils/ConfigParser.js:134:59)

Although yarn dist before. Will investigate Monday.

@yhuard
Copy link
Contributor

yhuard commented Jul 15, 2017

Hi @kalinchernev, you need to upgrade your Node.js version (see README for instructions with nvm). Object.values is a pretty new features (http://node.green/#ES2017-features-Object-static-methods-Object-values)

@degliwe degliwe assigned degliwe and unassigned degliwe Jul 17, 2017
yhuard
yhuard suggested changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@yhuard yhuard left a comment

Choose a reason for hiding this comment

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

Almost there!

framework/components/ecl-social-icons/ecl-social-icons.twig Outdated
{% if link is defined %}
<a
href="{{ link.target }}"
class="ecl-link ecl-icon-social {{ modifier }} {{ link.extraClasses }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

extra_attributes is missing (there could be some)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as well, though the link component is not really ready to use these at the moment.

framework/components/ecl-social-icons/package.json Outdated
"license": "EUPL-1.1"
"license": "EUPL-1.1",
"dependencies": {
"@ec-europa/ecl-base": "^0.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing dependency to ecl-links :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :)

yhuard
yhuard approved these changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@yhuard yhuard left a comment

Choose a reason for hiding this comment

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

Good enough for now ;)

@yhuard yhuard merged commit 9d4616d into master Jul 17, 2017
@yhuard yhuard deleted the feat/social-icons-inno-693 branch July 17, 2017 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhuard yhuard yhuard approved these changes

Assignees
No one assigned
Labels
tag: new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kalinchernev @yhuard @degliwe