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(profile-topbar): upgrade component - INNO-586 #238

Merged
merged 24 commits into from
Jul 11, 2017
Merged

feat(profile-topbar): upgrade component - INNO-586 #238

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
98a956f
feat(profile-topbar): start upgrade
kalinchernev Jul 3, 2017
8b01094
feat(profile-topbar): start upgrade
kalinchernev Jul 3, 2017
0d6d380
feat(profile-topbar): refactor context-navs template
kalinchernev Jul 4, 2017
9ccb997
feat(profile-topbar): merge latest master
kalinchernev Jul 4, 2017
0616700
Merge branch 'master' of github.com:ec-europa/europa-component-librar…
kalinchernev Jul 4, 2017
b0a05e5
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 4, 2017
73ef917
feat(profile-topbar): add refs
kalinchernev Jul 4, 2017
4bb93f3
Merge branch 'feat/profile-topbar-INNO-586' of github.com:ec-europa/e…
kalinchernev Jul 4, 2017
f72070a
feat(profile-topbar): static image for ref screenshots
kalinchernev Jul 4, 2017
aa5c04a
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 4, 2017
5b7e10c
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 4, 2017
1ed7668
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 5, 2017
2819481
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 7, 2017
a044c2e
feat(profile-topbar): include fixes
kalinchernev Jul 7, 2017
8bc1d02
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 10, 2017
716860f
feat(profile-topbar): improve documentation
kalinchernev Jul 10, 2017
2d5b07d
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 10, 2017
66176b9
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 10, 2017
98e0602
Merge branch 'master' into feat/profile-topbar-INNO-586
emeryro Jul 10, 2017
ba80fa6
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 10, 2017
bb2093e
feat(profile-topbar): invert spacing
kalinchernev Jul 10, 2017
7bd35bf
fix(ping-travis): empty
kalinchernev Jul 10, 2017
7fc7f64
fix(ping-travis): empty
kalinchernev Jul 10, 2017
50f9dda
Merge branch 'master' into feat/profile-topbar-INNO-586
kalinchernev Jul 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions framework/components/ecl-context-navs/ecl-context-navs.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,43 @@ module.exports = {
_demo: {
scripts: 'ECL.contextualNav();',
},
label: 'Label for contextual nav',
items: [
{
target: '#',
title: 'Item one',
},
{
target: '#',
title: 'Item two',
},
{
target: '#',
title: 'Item three',
},
{
target: '#',
title: 'Item four',
},
],
items_hidden: [
{
target: '#',
title: 'Item five',
},
{
target: '#',
title: 'Item six',
},
{
target: '#',
title: 'Item seven',
},
{
target: '#',
title: 'Item eight',
},
],
button_more: 'More',
},
};
53 changes: 13 additions & 40 deletions framework/components/ecl-context-navs/ecl-context-navs.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,27 @@
<div class="context-nav">
<span class="context-nav__label">Label for contextual nav</span>
<span class="context-nav__label">{{ label }}</span>
{% if items is defined and items is iterable %}
<div class="context-nav__items">
{% for item in items %}
<span class="context-nav__item">
<a href="#">Item one</a>
</span>
<span class="context-nav__item">
<a href="#">Item two</a>
</span>
<span class="context-nav__item">
<a href="#">Item three</a>
</span>
<span class="context-nav__item">
<a href="#">Item four</a>
</span>
</div>
</div>

<div class="context-nav">
<span class="context-nav__label">Label for longer list</span>
<div class="context-nav__items">
<span class="context-nav__item">
<a href="#">Item one</a>
</span>
<span class="context-nav__item">
<a href="#">Item two</a>
</span>
<span class="context-nav__item">
<a href="#">Item three</a>
</span>
<span class="context-nav__item">
<a href="#">Item four</a>
<a href="{{ item.target }}">{{ item.title }}</a>
</span>
{% endfor %}
{% endif %}
{% if items_hidden is defined and items_hidden is iterable %}
<div class="context-nav__expander">
<span class="context-nav__more-button">
More<span class="icon icon--down ecl-u-color-primary"></span>
{{ button_more }}
<span class="icon icon--down ecl-u-color-primary"></span>
</span>
<div class="context-nav__hidden">
{% for hidden_item in items_hidden %}
<span class="context-nav__item">
<a href="#">Item five</a>
</span>
<span class="context-nav__item">
<a href="#">Item six</a>
</span>
<span class="context-nav__item">
<a href="#">Item seven</a>
</span>
<span class="context-nav__item">
<a href="#">Item eight</a>
<a href="{{ hidden_item.target }}">{{ hidden_item.title }}</a>
</span>
{% endfor %}
</div>
</div>
{% endif %}
</div>
</div>
3 changes: 1 addition & 2 deletions framework/components/ecl-profile-topbars/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# Profile topbar

Defines the profile topbar component
TODO: this component's behaviour needs to be rewritten without jQuery.
Defines the profile topbar component.
23 changes: 0 additions & 23 deletions framework/components/ecl-profile-topbars/_profile-topbars.scss
View file
Open in desktop

This file was deleted.

55 changes: 54 additions & 1 deletion framework/components/ecl-profile-topbars/ecl-profile-topbars.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,59 @@
module.exports = {
title: 'Profile topbars',
label: 'Profile topbars',
status: 'planned',
status: 'ready',
tags: ['organism'],
context: {
_demo: {
scripts:
"document.addEventListener('DOMContentLoaded', function () { ECL.initExpandables(); });",
},
profile: {
avatar: 'http://lorempixel.com/output/business-q-c-160-160-10.jpg',
expandable: {
button: {
label: 'See details',
extraClass: 'ecl-expandable__button ecl-profile-topbar__pane-button',
extraAttributes: [
{
name: 'aria-controls',
value: 'ecl-profile-topbar__expandable-1',
},
{ name: 'aria-expanded', value: 'false' },
{ name: 'id', value: 'ecl-profile-topbar__expandable-button-1' },
{ name: 'type', value: 'button' },
],
},
expandable_body: {
extraAttributes: [
{
name: 'aria-hidden',
value: 'true',
},
{
name: 'aria-labelledby',
value: 'ecl-profile-topbar__expandable-button-1',
},
{ name: 'id', value: 'ecl-profile-topbar__expandable-1' },
{ name: 'class', value: 'ecl-profile-topbar__collapsible-area' },
],
},
},
details:
'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.',
Copy link
Contributor

Choose a reason for hiding this comment

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

As this content could be a complexe html structure (with, list, grid, etc.), I would rather use a block in twig file instead of a variable.

},
context_nav: {
label: 'Department',
items: [
{
target: '#',
title: 'Trade',
},
{
target: '#',
title: ' European Anti-Fraud Office',
},
],
},
},
};
31 changes: 31 additions & 0 deletions framework/components/ecl-profile-topbars/ecl-profile-topbars.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* ECL Profile Topbar
* @define profile-topbar
*/

.ecl-profile-topbar {
margin: 0;
}

// This is esentially .section__group which can be used when refactored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo in "essentially"

.ecl-profile-topbar__wrapper {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the puropose of this wrapper? Couldn't we use padding inside the component instead?
Here it adds margin outside the content of the component.

Copy link
Contributor Author

@kalinchernev kalinchernev Jul 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, it's a perfect candidate for a re-usable class. It's section__group as of current state of the repo. So, i added them as a wrapper and added specifically the section__group name so that it's found in the code base and replaced when the latter is upgraded.

The inverse padding will have to be added on a specific class in a similar way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what you say..
My remark was: instead of using external margins on a wrapper, we should probably use padding on the main .ecl-profile-topbar element.
That way, there won't be anything going outside the component.
And also if we do so, the wrapper itself becomes useless (no more css attributes on it)

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 think I made it right this time, the result is not totally the same, but hopefully acceptable.

margin-bottom: map-get($ecl-spacing, 'm');
margin-top: map-get($ecl-spacing, 'm');
}

.ecl-profile-topbar__pane {
height: 100%;
min-height: 160px;
position: relative;

> .ecl-profile-topbar__pane-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use ">" selector here I guess. There is a specific class on the button so css could be at same level as the other ones (no hierarchy)

bottom: 0;
margin-bottom: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This margin is useless as buttons already have margin:0 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍

position: absolute;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have use a display flex instead of position absolute.
That works well here, so we can keep it, but if the component grow larger (with several context-navs), there will be overlaps.

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 think the approach of relative -> absolute positioning is simple enough for this case. Flex would be a bit overkill in my opinion.

}
}

.ecl-profile-topbar__collapsible-area {
background-color: map-get($ecl-colors, 'grey-10');
overflow: hidden;
}
50 changes: 21 additions & 29 deletions framework/components/ecl-profile-topbars/ecl-profile-topbars.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,23 @@
<div class="profile_topbar">

<div class="container">
<div class="section__group row">
<div class="col-md-2 profile_topbar__image">
<img src="http://placehold.it/160x160"/>
</div>

<div class="col-md-10 profile_topbar__column">
<div class="context-nav group-context-nav field-group-context_nav"><span
class="context-nav__label">Department</span>
<div class="context-nav__items">
<span class="context-nav__item"><a href="#">Trade</a></span>
<span class="context-nav__item"><a href="#">European Anti-Fraud Office</a></span>
</div>
</div>

<button class="btn btn-collapse" data-target="#collapse" data-toggle="collapse">
<span class="toggling-text">Show</span>
<span class="icon icon--down ecl-u-color-primary"></span>
</button>
</div>
</div>
</div>

<div id="collapse" class="profile_topbar__expander">
<div class="container">
Expander content
</div>
<div class="ecl-profile-topbar">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the same structure for twig file as we started to use in other components:

  • api interface
  • declaration and initialisation of variables
  • markup

See featured-items component for instance.

<div class="ecl-container">
<div class="ecl-row ecl-profile-topbar__wrapper">
<div class="ecl-col-md-2">
<img src="{{ profile.avatar }}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing alt for image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, changed to {% include '@ec-europa/ecl-images' with profile.image %} :)

</div>
<div class="ecl-col-md-10 ecl-profile-topbar__pane">
{% include '@ec-europa/ecl-context-navs' with context_nav %}
{% include '@ec-europa/ecl-buttons' with profile.expandable.button %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Label of the button should change when expanding the text.
It will require an update of the expandable component; this can be done in a follow-up ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created INNO-682 as a follow-up.

</div>
</div>
</div>
<div
{% if profile.expandable.expandable_body.extraAttributes is defined %}
{% for attr in profile.expandable.expandable_body.extraAttributes %}
{% set attribute = attr.name ~ '="' ~ attr.value ~ '"' %}
{{ attribute }}
{% endfor %}
{% endif %}
>
<div class="ecl-container">{{ profile.details }}</div>
</div>
</div>
26 changes: 18 additions & 8 deletions framework/components/ecl-profile-topbars/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
{
"private": true,
"name": "@ec-europa/ecl-profile-topbars",
"version": "0.3.0",
"version": "0.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change version manually, it is handles automagically.

Copy link
Contributor Author

@kalinchernev kalinchernev Jul 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

reverted 👍

"description": "",
"main": "_profile-topbars.scss",
"style": "_profile-topbars.scss",
"main": "ecl-profile-topbars.scss",
"style": "ecl-profile-topbars.scss",
"author": "European Commission",
"license": "EUPL-1.1",
"peerDependencies": {
"@ec-europa/ecl-buttons": "^0.1.0",
"@ec-europa/ecl-context-navs": "^0.1.0",
"@ec-europa/ecl-icons": "^0.1.0"
}
"@ec-europa/ecl-buttons": "^0.7.1",
"@ec-europa/ecl-context-navs": "^0.3.1",
"@ec-europa/ecl-icons": "^0.5.1"
},
"publishConfig": {
"access": "public"
},
"repository": {
"type": "git",
"url": "git+https://github.com/ec-europa/europa-component-library.git"
},
"bugs": {
"url": "https://github.com/ec-europa/europa-component-library/issues"
},
"homepage": "https://github.com/ec-europa/europa-component-library#readme"
}
9 changes: 0 additions & 9 deletions framework/components/ecl-profile-topbars/profile-topbars.js
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion framework/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,3 @@ export * from './components/ecl-navigation/ecl-navigation-menus/megamenu';
export * from './components/ecl-tables/ecl-tables';
export * from './components/ecl-tabs/tabs';
// export * from './components/ecl-timelines/timelines';
// export * from './components/ecl-profile-topbars/profile-topbars';
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that the js was there to manage expandable block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it contained a console statement to refactor the js. Since the expandable component can be simply instantiated via the local package js file, I did that directly there without this file.

20 changes: 20 additions & 0 deletions test/functional/profile-toolbars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
describe('profile-toolbars', () => {
before(() => {
// Set viewport size
browser.setViewportSize({
width: 1400,
height: 600,
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshots could be smaller (but no need to change it now).

});

browser.pause(1000);
browser.url('ecl-profile-topbars.html');
});

// Normal state
it('should match the reference screenshot', () => {
const screenshots = browser.checkDocument({
name: 'profile-toolbars',
});
expect(screenshots).to.matchReference();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add accessibility tests too, as in other test files:

it('should be accessible', () => {
  const a11yReport = browser.runAxeCore('ecl-profile-topbar').value;
  expect(a11yReport).to.be.accessible;
});

});
Binary file added test/screenshots/reference/profile-toolbars/OS X 10.11_Safari_v9.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/screenshots/reference/profile-toolbars/Windows 10_Chrome_v56.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/screenshots/reference/profile-toolbars/Windows 7_Firefox_v46.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/screenshots/reference/profile-toolbars/Windows 7_IE_v11.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.