fix: Rendering the cursor before the shapes #4200
Conversation
| @@ -417,27 +417,38 @@ export const isSingleChildEqual = (nextChild: React.ReactElement, prevChild: Rea | |||
| return false; | |||
| }; | |||
|
|
|||
| type Priority = number; | |||
| type Index = number; | |||
| export const orderByPriorityAndIndex = <T>(items: [Priority, Index, T][]): [Priority, Index, T][] => { | |||
There was a problem hiding this comment.
I would prefer an implementation that uses a object, to benefit from named keys.
This would both make the code more readable as well as safer to use, i.e. one could not accidentally pass index and priority in the wrong order without noticing (both are number after all.)
| PolarGrid: { handler: this.renderPolarGrid, once: true, priority: 0 }, | ||
| PolarAngleAxis: { handler: this.renderPolarAxis, priority: 0 }, | ||
| PolarRadiusAxis: { handler: this.renderPolarAxis, priority: 0 }, | ||
| Customized: { handler: this.renderCustomized, priority: 1 }, |
There was a problem hiding this comment.
Why would we want to extend this with a number field, if really it can only be 0 or 1?
It seems to me that what instead care for is: Foreground vs Background.
I would always prefer types to be a simple as possible, and here number is needlessly open to extension. Soon, we might have someone add a new value, i.e. priority 2, or -1.
What do you think? Could we implement this with only a boolean flag? Something like isBackground: boolean?
|
We're trying to get rid of the renderMap, and element cloning. Can you think of a solution that uses a different approach? |
|
Also can we please have tests on the e2e expected behaviour? Why this change exists is to eventually sort elements in DOM, right? Let's test that. |
|
The point of the change is to render elements in the correct order because z index is not supported in SVGs. We'll have to do this somewhere, at some point, and currently there are a few bugs surrounding it (this one with the cursor rendering in front of bars for example). Since we can't prevent the user from putting things in random order, we will have to order things. Imo there could be more priority than just a Boolean value could hold. Not sure what the best approach is to do this but maybe it's best revisited after we refactor away the whole render map. Appreciate the effort @HHongSeungWoo! Might be something we need to think through more |
|
Is this something we have to solve for users? Is it acceptable solution to document that "what you render last will be on top"? If this is only for Cursor, can we move renderCursor call to be before children render? |
|
I would say we should ensure the things that should be in the back are in the back if we possibly can. There are a lot of issues relating to z axis stuff. Grid and Cursor being the two that I would say we should solve for if we can, other things maybe aren't such a big deal. |
|
^ Labels should also be readable |
|
If we choose the option to adjust the rendering order rather than documenting it, we can temporarily adopt this method until we refactor. However, this change could also be a breaking change for someone. |
|
I think this is now done with portals. On the 3.x branch only. |
Description
Added rendering priority and adjusted the rendering order.
Related Issue
#3990
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: