feat(profile-topbar): upgrade component - INNO-586 #238
Conversation
…y into feat/profile-topbar-INNO-586
kalinchernev
changed the title
kalinchernev
changed the title
…uropa-component-library into feat/profile-topbar-INNO-586
| </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 %} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Created INNO-682 as a follow-up.
| <div class="container"> | ||
| Expander content | ||
| </div> | ||
| <div class="ecl-profile-topbar"> |
There was a problem hiding this comment.
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.
| }, | ||
| }, | ||
| 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.', |
There was a problem hiding this comment.
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.
| margin: 0; | ||
| } | ||
|
|
||
| // This is esentially .section__group which can be used when refactored. |
There was a problem hiding this comment.
Small typo in "essentially"
| } | ||
|
|
||
| // This is esentially .section__group which can be used when refactored. | ||
| .ecl-profile-topbar__wrapper { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I think I made it right this time, the result is not totally the same, but hopefully acceptable.
| "name": "@ec-europa/ecl-profile-topbars", | ||
| "version": "0.3.0", | ||
| "version": "0.4.0", |
There was a problem hiding this comment.
No need to change version manually, it is handles automagically.
| @@ -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'; | |||
There was a problem hiding this comment.
I assume that the js was there to manage expandable block.
There was a problem hiding this comment.
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.
| // Set viewport size | ||
| browser.setViewportSize({ | ||
| width: 1400, | ||
| height: 600, |
There was a problem hiding this comment.
Screenshots could be smaller (but no need to change it now).
| <div class="ecl-container"> | ||
| <div class="ecl-row ecl-profile-topbar__wrapper"> | ||
| <div class="ecl-col-md-2"> | ||
| <img src="{{ profile.avatar }}"/> |
There was a problem hiding this comment.
thanks, changed to {% include '@ec-europa/ecl-images' with profile.image %} :)
| name: 'profile-toolbars', | ||
| }); | ||
| expect(screenshots).to.matchReference(); | ||
| }); |
There was a problem hiding this comment.
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;
});
|
Updates applied, ready for another review round. |
For
@ec-europa/ecl-context-navsthere is change only in the template to be reusable, but I haven't changed the CSS as it's not in scope of this ticket.