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.

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

refactor(page-header): update page header according to DT spec - INNO-441 #153

Merged
merged 10 commits into from
May 31, 2017

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented May 30, 2017

Page header updated.
Code cleaned up. There were lots of unused code, maybe there are some other use cases, but I didn't found them in DT documentation.

Breadcrumb and meta components will have to be updated later (I kept them as is for now)

Copy link
Contributor

@kalinchernev kalinchernev left a comment

Choose a reason for hiding this comment

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

I couldn't complete the full set of changes, though it seems few things are missing such as highlights with text, background images


.page-header {
margin: 0;
padding: 0 0 1.5em;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why removing this?

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 didn't have any reason to keep it, as it didn't change the display

@emeryro
Copy link
Contributor Author

emeryro commented May 31, 2017

@kalinchernev I based my implementation on DT specifications (on their wiki), not on DTT implementation (even if I sticked to the design).
There is no references to background images or other display in the specifications...

@emeryro
Copy link
Contributor Author

emeryro commented May 31, 2017

Just discussed with @degliwe and @yhuard, I will add the other variants

@emeryro
Copy link
Contributor Author

emeryro commented May 31, 2017

Missing modifiers have been added.
Ready for review

@kalinchernev kalinchernev self-assigned this May 31, 2017
@kalinchernev kalinchernev force-pushed the refactor/page-header-INNO-441 branch from 2eca4cc to d3ebd01 Compare May 31, 2017 09:23
@degliwe degliwe self-assigned this May 31, 2017
Copy link
Contributor

@degliwe degliwe left a comment

Choose a reason for hiding this comment

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

  • the border bottom on the breadcrumb should not be edge to edge but with a padding.
  • highlight header: the padding is noticeably different (it should be one step bigger)

@kalinchernev kalinchernev force-pushed the refactor/page-header-INNO-441 branch from d3ebd01 to 072cf7e Compare May 31, 2017 09:50
Copy link
Contributor

@kalinchernev kalinchernev left a comment

Choose a reason for hiding this comment

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

It's good enough for me

…-europa/europa-component-library into refactor/page-header-INNO-441

# Conflicts:
#	framework/components/ecl-page-headers/_page-headers.scss
#	framework/components/ecl-page-headers/corporate/corporate.twig
#	framework/components/ecl-page-headers/improved-basic/improved-basic.twig
#	framework/components/ecl-page-headers/improved-complete/improved-complete.twig
@emeryro
Copy link
Contributor Author

emeryro commented May 31, 2017

Paddings have been fixed.
@degliwe good for you too?

Copy link
Contributor

@degliwe degliwe left a comment

Choose a reason for hiding this comment

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

almost done.
[improve-complete] breadcrumb border bottom is different than the other variant

@emeryro
Copy link
Contributor Author

emeryro commented May 31, 2017

improve-complete has been fixed

@degliwe degliwe merged commit 41fcf62 into master May 31, 2017
@degliwe degliwe deleted the refactor/page-header-INNO-441 branch May 31, 2017 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants