-
Notifications
You must be signed in to change notification settings - Fork 35
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(tabs): add tabs component - INNO-617 #201
Conversation
# Conflicts: # test/screenshots/reference/navigation/lists/default/OS X 10.11_Safari_v9.png # test/screenshots/reference/navigation/lists/default/Windows 10_Chrome_v56.png # test/screenshots/reference/navigation/lists/default/Windows 7_Firefox_v46.png # test/screenshots/reference/navigation/lists/default/Windows 7_IE_v11.png # test/screenshots/reference/navigation/lists/tabs/OS X 10.11_Safari_v9.png # test/screenshots/reference/navigation/lists/tabs/Windows 10_Chrome_v56.png # test/screenshots/reference/navigation/lists/tabs/Windows 7_Firefox_v46.png # test/screenshots/reference/navigation/lists/tabs/Windows 7_IE_v11.png # test/screenshots/reference/navigation/menus/OS X 10.11_Safari_v9.png # test/screenshots/reference/navigation/menus/Windows 10_Chrome_v56.png # test/screenshots/reference/navigation/menus/Windows 7_Firefox_v46.png # test/screenshots/reference/navigation/menus/Windows 7_IE_v11.png
@@ -0,0 +1,24 @@ | |||
{ | |||
"name": "@ec-europa/ecl-navigation-lists", |
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.
tabs
, small thing
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.
How did I miss that? 😄 Thanks!
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.
I don't see any issues, only really minor comments
"main": "_ecl-tabs.scss", | ||
"style": "_ecl-tabs.scss", | ||
"peerDependencies": { | ||
"@ec-europa/ecl-links": "^0.1.0", |
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.
The version of is 0.5.0
at the moment, could be a bit higher
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.
At some point, we will have to rethink the way we handle the dependencies between the packages. Right now, we only use peerDependencies
which are more informational / not strict enough. I guess that we will need to switch to dependencies
, but it will also slow the bootstrap phase since lerna will have to link all the dependencies between them correctly...
// Heavily inspired by the tab component from https://github.com/frend/frend.co | ||
|
||
// Query helper | ||
const q = (el, ctx = document) => [].slice.call(ctx.querySelectorAll(el)); |
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.
Maybe add it a bit more globally so all components can benefit?
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.
I've also thought about it. Making a small set of utilities provided with ecl-base
. Will do it :)
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 in ecl-base/helpers/dom.js
@@ -0,0 +1,20 @@ | |||
<div class="ecl-tabs"> |
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.
A question here. When the D8 (potentially) uses the templates from our packages, is it that they can use it better if we provide the information in the templates via context?
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.
Absolutely. The template has to be rewritten to introduce some logic into it. So does a couple of other components. We can discuss that tomorrow :)
@@ -0,0 +1,5 @@ | |||
// Query helper | |||
export const queryAll = (selector, context = document) => |
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.
Thanks for this!
TODO: