Comments (13)
Calling render phase of a component will force to recreate its nodes, it's the same way in YUI if HTML_PARSER is not used. Few options you have here: 1) Call decorate instead of render; 2) Use event delegation in the component bounding box, that way events are preserved regardless the node instance; 3) Wait for incremental dom renderer be ready.
from metal.js.
Hey @eduardolundgren,
- Already tried with decorate. The result is the same (afaict)
- This would involve making blind changes in code without an apparent reason. Code that attaches the listener and code that renders the dropdown are unaware of each other...
- This will probably mean we won't be able to push metal-components in portal until then 😢
from metal.js.
We've talked about this offline, and confirmed that this is the expected behavior. We'll work on fixing the problematic use cases on portal side. Closing this then.
from metal.js.
IMHO having listeners attached to elements before the main component framework kicks in is wrong on an awful lot of levels.
@jbalsas What actually is the case?
from metal.js.
Hey @yuchi, not sure how to put this in here, but just for your sake I'll try in pseudo-code 😉
<liferay-ui:icon-menu>
<%-- begin start.jsp --%>
<ul>
<%-- end start.jsp --%>
for (item in items) {
<%-- begin render item i %-->
<button id="myButton">
<script>$('#myButton').on...</script>
<%-- end render item i %-->
}
<%-- begin end.jsp --%>
</ul>
<script>
new Dropdown({...}).decorate();
</script>
<%-- end end.jsp --%>
</liferay-ui:icon-menu>
It's not that I don't agree with you, it's just that code might (and does) get laid out this way causing issues. Keep in mind that menu items are independent and agnostic of who and how is using them.
from metal.js.
The issue is obviously on the fact that there’s inline JS manipulating/accessing the DOM in there.
Either use a plain old synthetic event for those kind of things or delegate everything to the component.
Managing click
manually is inherently wrong and error prone.
from metal.js.
@yuchi, obviously, but I wouldn't flat out say this code is to blame.
As wrong as it may be, this code is contributed by a small module which is just being told to render an item. It's a little bit unfair to put the blame on it when we're moving the floor under its feet...
Also, the code knows nothing about being wrapped in a specific component, so the only safe delegate to do could be to the body, and as such, it will need to make sure to clean up on spa navigation.
We'll definitely try to fix this specific part of code, but that doesn't mean that we might be breaking other code out there :(
from metal.js.
It must know it will be transformed.
What about using attributes? I'm getting less and less afraid of using onclick
and siblings. Those define clearly a single responder and a delegate at the same time, and they are retainable in the decoration phase without slowing it.
from metal.js.
Not necessarily, because it might happen that it doesn't get transformed... one could switch the wrapping menu component implementation to, let's say
- A.Dropdown -> does decorate, does not mutate dom
- Bootstrap drodpown -> does nothing
- metal.Dropdown -> mutates dom
Ideally, you'd still want the independently contributed items to work under all those scenarios, agnostic of the wrapping implementation.
Granted, the end result will be the same; we need to specify which is the recommended way of writing those little pieces so things like this doesn't happen. 😉
from metal.js.
@jbalsas If markup is correct metal.Dropdown will not mutate. When incremental dom support becomes ready on metal, it will be true for any matching node by the key identifier of incremental dom, it's more flexible since the markup doesn't need to be exact the same and instead just partially equal.
from metal.js.
This is where React shines. Even more in universal—client and server—mode.
// final consumer
import { IconMenu, IconMenuItem } from 'liferay-components/icon-menu';
const DLFileEntryMenu = props => (
<IconMenu>
<IconMenuItem label="delete" onClick={ props.onDelete } />
</IconMenu>
);
// liferay-components/icon-menu.js
export let IconMenu => props => ( /* default impl here */ );
export let IconMenuItem => props => ( /* default impl here */ );
// we could wrap the delegate here to have pre and after hooks
export const delegate = ({ menu, item }) => {
IconMenu = menu;
IconMenuItem = item;
};
// another implementer
import { delegate } from 'liferay-components/icon-menu';
export default const MyIconMenu = props => ( /* my impl here */ );
export default const MyIconMenuItem = props => ( /* my impl here */ );
delegate({
menu: MyIconMenu,
item: MyItemMenuItem
});
from metal.js.
Also (without getting OT) what if the implementation completely changes the interaction of an object?
What if instead of using clicks it will use mouse or touch swipes?
The contract shouldn't be on events but on attributes/properties of the element. In this specific case we have href
exactly for this.
from metal.js.
@eduardolundgren, yeah, I know, it was just an example, not blaming metal here at all... we're going to get so much better with incremental dom in place
@yuchi, that's what I mean, the contract says, give me something, I'll print it. It doesn't advertise it might mutate the nodes and you shouldn't do this or that. As Eduardo points out we might be producing the wrong markup there, but the contract doesn't state "please use this markup so nodes are properly reconciled and not re-rendered" either. Again, I'm not saying it's right, just what it is... and for sure, we're going to change it 😉
from metal.js.
Related Issues (20)
- Add source maps to compile scripts
- Implement functionality similar to componentDidCatch from React
- Allow iterables, not just arrays, as child elements
- TypeScript definitions HOT 2
- [incremental-dom-string] Bring in incremental-dom-string from metal-plugins
- [infra] Update to [email protected]
- [metal-incremental-dom] Update incremental-dom-string dependency to 0.0.3
- Metal Dom uses `document` but in Node there is no such thing HOT 2
- Typo on tutorials
- [metal-dom] domNamed.contains isn't working on IE11 when I pass document as first argument
- dom.contains method does not support SVG elements in IE11
- Create a metal-soy-redux style library HOT 1
- Config does not validate required nested attributes
- Replace Object.assign with an internal implementation
- Package.json spdx BSD format HOT 1
- `npm run prepublish error` when running `npm run lerna` HOT 2
- JWT addon in Travis CI was discontinued on 17 Apr 2018
- Failing HTML entity unescaping test in HTML2IncDom
- Please enable automated monthly build/test on Travis CI via cron jobs
- Is metal.js dead? HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from metal.js.