Remove dependencies of metal-dropdown
#146
Conversation
|
Just started reviewing :) |
Sent from 
| * @inheritDoc | ||
| */ | ||
| detached() { | ||
| super.attached(); |
There was a problem hiding this comment.
Lifecycle methods are just defined in Component, but there's nothing inside. I guess we can get rid of this super call.
https://github.com/metal/metal.js/blob/master/packages/metal-component/src/Component.js#L283
| * @inheritDoc | ||
| */ | ||
| attached() { | ||
| super.attached(); |
There was a problem hiding this comment.
Lifecycle methods are just defined in Component, but there's nothing inside. I guess we can get rid of this super call.
https://github.com/metal/metal.js/blob/master/packages/metal-component/src/Component.js#L217
| [Align.LeftCenter]: 'dropleft', | ||
| }; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
SF: Method alphabetically ordered
| @@ -2,17 +2,153 @@ import 'clay-button'; | |||
| import 'clay-checkbox'; | |||
There was a problem hiding this comment.
SF: Order imports alphabetically
| expanded: Config.bool().value(false), | ||
| expanded: Config.bool() | ||
| .value(false) | ||
| .internal(), |
There was a problem hiding this comment.
This is not internal
| * @return {!Object} | ||
| * @protected | ||
| */ | ||
| valueClassMapFn_() { |
There was a problem hiding this comment.
classMap was used in metal-dropdown soy template. Since we're not using this param in our templates we can get rid of this method.
| * @type {!Object} | ||
| * @default valueClassMapFn_ | ||
| */ | ||
| classMap_: Config.validator(object) |
There was a problem hiding this comment.
classMap was used in metal-dropdown soy template. Since we're not using this param in our templates we can get rid of this.
| */ | ||
| syncExpanded(expanded) { | ||
| if (expanded && this.alignElementSelector) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
We should try avoiding to disable line linting. In this case we can just split the line in two:
let alignElement = this.element.querySelector(
this.alignElementSelector);
There was a problem hiding this comment.
@carloslancha I had done that, but the prettier is leaving everything inline, had to add to not break in the CI.
There was a problem hiding this comment.
We merged prettier/prettier-eslint#141 yesterday and a new version of prettier-eslint was auto-released. It should solve the issue.
@matuzalemsteles, can you try updating the dependencies to see if this goes away?
There was a problem hiding this comment.
Hey @jbalsas, it is necessary that prettier-eslint-cli update the version of prettier-eslint to 8.2.2.
There was a problem hiding this comment.
@Robert-Frampton, could you take care of that? I thought they were synced somehow :(
There was a problem hiding this comment.
@jbalsas for some reason the npm run checkFormat is failing.
There was a problem hiding this comment.
It's because some files are out of format. See the log:
packages/clay-alert/src/ClayAlert.js
packages/clay-charts/src/ChartBase.js
packages/clay-collapse/src/ClayCollapse.js
packages/clay-dropdown/src/items_validator.js
packages/clay-modal/src/ClayModal.js
packages/clay-collapse/src/__tests__/ClayCollapse.js
packages/clay-navbar/src/__tests__/ClayNavbar.js
success formatting 7 files with prettier-eslint
If you run npm run format locally, would you probably see those changes. The checkFormat task is there to ensure nothing gets committed with the wrong format.
Can you run that and see what's changing? You can run the npm run checkFormat task before sending this over to see if some files need formatting.
There was a problem hiding this comment.
This was due to a yarn upgrade prettier-eslint-cli which upgraded prettier-eslint to 8.2.2 and had to make some changes. So as not to mess up the yarn.lock tree, I think we'd better keep it out of this, until prettier-eslint-cli is using 8.2.2. what do you think ?
| * @type {string|number} | ||
| * @default Align.BottomLeft | ||
| */ | ||
| position_: Config.setter('setterPositionFn_') |
There was a problem hiding this comment.
If we set this internal, since we're not setting it anywhere, we don't really need it. Remove this, setterPositionFn_ and validatePosition_ methods and replace line 105 with:
this.alignedPosition_ = Align.align(
bodyElement,
alignElement,
Align.BottomLeft
);
| import Dropdown from 'metal-dropdown'; | ||
| import Soy from 'metal-soy'; | ||
| import {Align} from 'metal-position'; | ||
| import {core, object} from 'metal'; |
There was a problem hiding this comment.
No needed after removing all stuff
|
Hey @carloslancha, Removes the methods that were not being used and changed the way I was adding the |
matuzalemsteles
force-pushed
the
remove-dropdown
branch
2 times, most recently
from
83ba2b0 to
1c39a5c
Compare
|
Just started reviewing :) |
|
Merged, thx! :) |
This resolves #145.
I just put the
metal-dropdowncode inclay-dropdownand remove some unnecessary things.