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
feat(page-header): add site-wide components - FRONT-3546 #2337
Conversation
papegaill
commented
Feb 15, 2022
•
edited
edited
- EC: https://ecl-preview-2337--europa-component-library.netlify.app/playground/ec/?path=/story/components-site-wide-page-headers--core
- EU: https://ecl-preview-2337--europa-component-library.netlify.app/playground/eu/?path=/story/components-site-wide-page-headers--core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use another image at least for the core page header (blue on blue isn't very nice)
(there are several images already available on the server, just change the number at the end https://inno-ecl.s3.amazonaws.com/media/examples/example-image2.jpg)
export const MetaTitle = (args) => | ||
pageHeader(prepareData(demoMetaTitleContent, args)); | ||
|
||
MetaTitle.storyName = 'meta-title'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having multiple stories here we should follow the same logic as other component: a single story and controls for optional elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
import { withNotes } from '@ecl/storybook-addon-notes'; | ||
import withCode from '@ecl/storybook-addon-code'; | ||
import { correctSvgPath } from '@ecl/story-utils'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same remark as on the footer: there is no EU page header harmonised?
This is strange as there is a site header...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const argTypes = {}; | ||
|
||
argTypes.breadcrumb = { | ||
name: 'breadcrumb', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep the same kind of structure/naming for the optional controls:
show_breadcrumb, and put in an "optional" group
(check the content-item for instance)
{% spaceless %} | ||
{# | ||
Parameters: | ||
- "variant" (string) (default: '') core, standardised or harmonised |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having variant for the families, we should have two possible displays ("positive" and "negative")
then one of the other will be used to create a family specific page header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/implementations/twig/components/page-header/page-header.test.js
Outdated
Show resolved
Hide resolved
/* | ||
* Core - Negative Variant | ||
*/ | ||
.ecl-page-header--core { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be named "ecl-page-header--negative"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, we will also change the breadcrumb include in the same way when we redo that component 👍
@@ -0,0 +1,12 @@ | |||
module.exports = { | |||
title: 'Page title', | |||
description: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having multiple data files we could have just one containing everything and handle it in the stories to display some elements or not.
that's how we handled latest component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/website/src/pages/ec/components/site-wide/page-header/docs/code.mdx
Outdated
Show resolved
Hide resolved
…-component-library into feat/FRONT-3546
…-component-library into feat/FRONT-3546
…-component-library into feat/FRONT-3546