-
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
refactor(tables): update tables - INNO-517 #148
Conversation
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 markup is better, cleared.
The js part works without any problems. (I don't have IE locally though)
Maybe a small example would be useful for people who don't know the details of calling a method of Europa module:
/**
* @file
* Example of using responsive table from ECL.
* You do not need jQuery.
*/
(function () {
Drupal.behaviors.ECL = {
attach: function (context, settings) {
Europa.eclTables();
}
};
}());
Good improvements!
*/ | ||
|
||
// eslint-disable-next-line import/prefer-default-export | ||
export function eclTables() { |
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.
What if you want to apply this transformation not on all tables but only on a subset? My two cents: you could add an optional parameter eclTables(elements = null)
and then reuse it const tables = elements === null ? document.getElementsByClassName('ecl-table') : elements;
. Or you could pass the the query selector and then call querySelectorAll
instead of getElementsByClassName
:)
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.
It may be an improvement.
Anyway, the code will have to be updated, as now it isn't optimal (too many DOM browsing that should be put in variables).
I'd say, we merge the branch as is, and we create a ticket for this, with your comment (and Kalin's remark too). If we have time to deal with it before thursday, great, in not we have at least something working.
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.
Solution: we create a follow-up ticket and merge this PR.
Tables updated from DTT implementation (no DT specifications).
Responsive display is managed both by javascript and css (javascript converted from jquery to vanilla).
Code should probably be optimized in the future (once we have real specifications)