-
Notifications
You must be signed in to change notification settings - Fork 55
RFC: Layout and ItemLayout render a big amount of nested DOM nodes #241
Comments
I would rather propose to focus on correctness aspects while being at the stage we are now, rather than trying to optimize. But even speaking of whether it is a problem for now - to me it seems that it is not, as DOM tree that is currently rendered seems to be appealing in terms of semantics each So, just to reiterate - lets consider to close it for now, and if we would have a serious problem related to the amount of DOM nodes rendered, we will return to it - at the same time, probably, we would be able to introduce simplier techniques to handle this problem (like relying on CSS layout tools), rather than tweak the component's logic for it. |
I am on the same page when it comes to the fact that a correct approach is vital, but I am a strong believer that the approach should be simple and optimal, especially in these early stages. I don't agree with closing this, but rather add some proposal to it first and see if someone comes up with something feasible in terms of how correct, optimal simple and time consuming (from dev implementation POV) the proposals are. It's also a lot better to tackle this now rather than later when it could be spread on a lot of components. The side effects would be so much bigger making a change like this later. @levithomason showed us a nice POC on how the DOM tree size can be drastically reduced using CSS grid for very simple elements (I think for Imagine having that element tens or hundred of times in your app (the case of buttons, chat messages, labels, etc), we'd have hundreds of unnecessary nodes being rendered and a potential source of:
|
Work in progress here, #401. The idea is to see if CSS Grid layouts can be used to replace most of our Layout needs. |
(pasting the discussion we had some time ago) I did a small prototype to find out if grids with templates will work from accessibility point of view if they wrap focusable elements. It works correctly only if the template order is the same as the order in DOM. If you use the template to switch the order, both FocusZone as well as virtual cursors of screen readers break. You can see it in proto/css-grid-focus branch (CSS Grid prototype in the menu) So we can use templates, but our API should not allow to reposition areas - they always need to be in the same order as they are in DOM. Usual ways of changing the tab order (tabIndex >= 1 or responding to TAB keypress events) work only partially as they do not influence the order in which screen readers order the tabbable elements. |
closing this one as there is a dedicated issue created for 'problematic' components, #525 |
Layout and ItemLayout render a big amount of nested DOM nodes
Steps
I was trying to reuse and
ItemLayout
(andLayout
) Stardust components to re-write theChat.Message
component since it looked that I can leverage the logic provided byItemLayout
props (media
,endMedia
,header
,headerMedia
, etc) to addauthor
anddate
props toChat.Message
.However, I realized these layout components render a big amount of redundant DOM nodes; here is an example for a very simple
Chat.Message
implemented withItemLayout
:Example code:
Screenshot:
Rendered DOM:
Implementation:
Issue:
Expected Result
The DOM tree has a reasonable depths (let's say a max of 4 nodes).
Actual Result
The DOM tree has a depth of 6 just for this basic example!
Proposed solution
Add better conditions to
Layout
andItemLayout
components in order to prevent additional DOM elements in the tree when a certain rendered area (start, main, end, header, media, etc) is represented by:ItemLayout
/Layout
componentWhat we do now is to add a wrapper DOM element (
div
usually) to wrap content for a layout slot. We can use the single node itself for that and apply the layouting styles directly.Version
0.5.2
The text was updated successfully, but these errors were encountered: