Skip to content
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: reorganise layers #10

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Mar 26, 2020

UPDATE:
Now ready for review.


This PR is not done yet, but does illustrate how I would like to rearrange the overlay components for v5. When done, this PR would solve https://github.com/dhis2/ui-core/issues/760. I've tackled things slightly differently than I wrote in that issue, see below:

  • Layer covers the window or body and takes care of stacking (z-index). You can also give this component a semi-transparent background by adding the translucent prop. It's also possible to create a "non-blocking" layer by setting pointerEvents="none". And finally, you can attach an onClick handler.
  • ComponentCover covers the component, provided the parent has a non-static position. The other props are identical to the Layer (i.e. onClick, translucent, pointerEvents)
  • CenteredContent: aligns its content vertically and horizontally.

The following components would be deprecated and replaced by the new ones above:

  • Backdrop
  • Layer (from LayerContext.js)
  • ScreenCover

Tasks:

  • Improve Layer component
  • Add tests for layer behaviour
  • Refactor DropdownButton to use Layer
  • Refactor SplitButton to use Layer
  • Refactor Select to use Layer
  • Fix Select regressions reg. selection/deselection and size/position updates
  • Add tests for Select regressions
  • Refactor Modal to use Layer and CenteredContent
  • Remove redundant components and tests (Backdrop, LayerContext and ScreenCover)
  • Add/Update demos and docs
  • Squash commits

@HendrikThePendric HendrikThePendric force-pushed the refactor/reorganise-layers-and-overlays branch from cee228e to 61439f0 Compare April 1, 2020 12:53
@HendrikThePendric HendrikThePendric force-pushed the refactor/reorganise-layers-and-overlays branch from 3bf8d2f to 1a5e355 Compare April 9, 2020 14:41
@HendrikThePendric HendrikThePendric self-assigned this Apr 9, 2020
@HendrikThePendric HendrikThePendric marked this pull request as ready for review April 9, 2020 14:42
@HendrikThePendric HendrikThePendric requested a review from a team as a code owner April 9, 2020 14:42
@HendrikThePendric HendrikThePendric added the breaking change Fixing this will break the existing API label Apr 9, 2020
@HendrikThePendric HendrikThePendric changed the title refactor: reorganise layers wip refactor: reorganise layers Apr 9, 2020
@HendrikThePendric HendrikThePendric force-pushed the refactor/reorganise-layers-and-overlays branch from f356774 to a514189 Compare April 14, 2020 18:55
Comment on lines +38 to +44
Scenario: The user clicks an option twice to select and deselect it
Given a MultiSelect is rendered to which options can be added
And the MultiSelect is open
When an option is clicked
Then the clicked option is selected
When the selected option is clicked again
Then the previously selected option is deselected
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario was added to verify a problem in the MultiSelect is not present, see #10 (comment)

@@ -15,7 +15,6 @@ const onDisabledClick = (_, e) => {

const createHandler = ({ isActive, onChange, selected, value }) => (_, e) => {
e.stopPropagation()
e.preventDefault()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was removed to fix the following bug:

  • Clicking an option Checkbox once worked correctly
  • But after that first click it would always require two clicks to toggle an option Checkbox

After consulting @ismay, a test was added to verify the bug is gone, see #10 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to look into why this was going wrong exactly. I wonder also how many of the stopPropagation's and preventDefault's we could get rid of btw, but that's maybe a different issue.

When('an option is clicked', () => {
cy.contains('option one').click()
// Wait until the DOM is painted
cy.wait(100)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worries me. I could tell from the snapshots I was dealing with a timing issue. Initially I added a timeout of 1 ms, which I thought was justifiable as a poor-man's substitute for requestAnimationFrame. This worked fine locally, but after pushing this commit, and upgrading some dependencies, the tests in GitHub actions started failing. I have only been able to resolve these issues this way, but I fear there is an underlying reason why these tests are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worries turned out to be justified. All tests connected to the popper's resize-observer behaviour were flaky. Some test runs would pass and others would fail. There didn't seem to be a pattern in it. I've spent most of today trying to find a decent solution for this, either in the component or in the form of a cypress helper.....

Since the behaviour of the component actually seemed correct when testing manually and when inspecting the DOM snapshots in cypress, I chose to try and fix this on the cypress side. I've come up with a helper function that uses a ResizeObserver itself. Since implementing that I've had a series successful test runs, so hopefully the problem is now resolved....

function waitForResizeObserver(callback) {
cy.window().then(() => {
const id = 'resize-observer-callback-executed'
const oldNode = document.getElementById(id)
// Cleanup
if (oldNode) {
oldNode.parentNode.removeChild(oldNode)
}
cy.get('[data-test="dhis2-uicore-select"]').then($el => {
const el = $el[0]
const observer = new ResizeObserver(() => {
// Create element to wait for when resizeObserver callback is executed
const newNode = document.createElement('div')
newNode.setAttribute('id', id)
el.parentNode.appendChild(newNode)
})
observer.observe(el)
callback()
// Wait for element and DOM redraw
return cy.get(`#${id}`).then(() => cy.wait(1))
})
})
}

When('the window is resized to a greater width', () => {
cy.viewport(1200, 660)
// Wait until the DOM is painted
cy.wait(100)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar/identical issue as the one mentioned in this #10 (comment). In both cases the reference element changes size and the ResizeObserver picks up on these changes to update the popper element. It seems cypress asserts just before the ResizeObserver kicks into action..... 🤔

level: propTypes.number,
position: propTypes.oneOf(['absolute', 'relative', 'fixed']),
pointerEvents: propTypes.oneOf(['all', 'none']),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables "non-blocking" layers, which could theoretically be handy if you would want to visually "layer" elements on top of one another, but still allow interaction with the underlying layer. There'd be a gotcha here, which is that the layer content would need to make sure to set pointer-events: all; again.

bottom: 0;
left: 0;
z-index: ${layers.applicationTop};
pointer-events: ${pointerEvents};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@ismay ismay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is a difficult topic in my opinion. I feel like every time when we're talking about these z-index issues, I need to read up again on the exact intricacies of z-index. So I've looked up articles on it, see how others are dealing with this. I don't know if I'm the only one who has that, but I'll just put what I found here, might help:

Basics

  • So there's an article on Smashing Magazine about it here.

Short summary:

  • z-index determines paint order. The higher the number, the later it's painted. If z-index values are the same, then the later it’s in the source, the later it’s painted.
  • z-index is hierarchical. So an element can create a stacking context, which becomes the root for z-index values of its descendants.

To elaborate a bit on the stacking context:

If an element is said to create a stacking context, it creates a basis for its children’s z-index values, so they’re never compared with anything outside the stacking context when determining paint order. To put it another way, when an element creating a stacking context is painted, all its children are painted right after it and before any of its siblings.

So basically, everything in a local stacking context is only compared to what's in the local stacking context. So painted as a single image, in a way.

The solution proposed in this article is to create a local stacking context for every component:

The answer is a convention that eliminates the need for guesswork, and it’s the following: changing z-indices within a component should only affect that component, and nothing else. To put it differently, when dealing with z-index values in a certain CSS file, we should ideally only concern ourselves with other values in that same file.

Achieving it is easy. We should simply make sure that the root of every component creates a stacking context. The easiest way to do it is to give it position and z-index values other than the default ones (which are static and auto, respectively).

You only have to do the above if an element within a component needs a z-index value other than auto. It does have some drawbacks, super complicated overlaps can't be created this way.

Hackernoon

An article on hackernoon by David Gilbertson recommends that you distinguish between local and global z-indexes. With global meaning that you:

care about how two elements stack relative to each other when they’re both on the screen at the same time. Since the relationship is important, the only sensible thing to do is to record the z-indexes for each of these elements in the same place. For global z-indexes you're setting z-index on elements in the ‘root stacking context’.

For z-indexes that you consider local he's:

talking about setting the z-index of an element within a new stacking context. When an element creates a new stacking context, no child element will be able to render on top of elements elsewhere on the page. Think of it as ‘scoping’ your z-indexes. (This might make it more confusing, but I like to think of it as turning a z-index of 999 into 0.999.)

There are multiple ways to create a new stacking context. Either by:

  • Setting position: relative and z-index: 0 on an element (this seems the nicest one)
  • Setting transform: translate(0) (has drawbacks)
  • Setting any opacity less than 1 (obvious drawback)

So what he's advising is to:

  • Centralize all z-indexes that need to interact with eachother, so you can order them as necessary
  • Create a local stacking context for everything that only needs to internally layer things, so that it doesn't conflict with the outside stacking

Uber's component lib

Uber's component library has a layer component as well, https://baseweb.design/components/layer:

Every UI has stackable layers and it's important to have control over how the layers play together. Some examples of the stackable layers are tooltips, modals, popovers, select dropdowns or menus. The issues that come up along the layers usage are z-index and visibility handling, what layer goes on top. There are many other layer related issues that include focus management, hover management, keyboard navigation, click events and hotkeys usage.

They have a provider that provides a LayersManager. The app is mounted in one div, and the layers in an adjacent one. The interesting thing is that they don't use z-index at all:

With layers there is no need for z-index css property usage as layers completely rely on the stacking context ... In general try to avoid using z-index in your component based application. If you need to have a z-index added to some element on a page make sure it's within an ordered stacking context.

I wonder if we could take anything from their approach. They seem to be solving a very similar problem.

P.S.

Sorry to just drop this comment bomb in the PR. I just felt like I needed to read up a bit on z-index, and was wondering what the general recommendations are, and thought I'd share what I'd found. I hope it helps when reviewing our Layers approach. I do hope that we can stick as closely as possible to the 'native' z-index behaviour.

@HendrikThePendric
Copy link
Contributor Author

Sorry to just drop this comment bomb in the PR. I just felt like I needed to read up a bit on z-index, and was wondering what the general recommendations are, and thought I'd share what I'd found. I hope it helps when reviewing our Layers approach. I do hope that we can stick as closely as possible to the 'native' z-index behaviour.

@ismay thanks for sharing all the info in your comment. I will dive into the details tomorrow. For now I'd like to reassure you that this current implementation stays very close to 'native' z-index behaviour. We are leveraging the stacking context as much as possible, with really only one exception:
If a layer with a higher z-index is is created from a an existing layer with a lower z-index, than that layer will not be appended to the parent, but to the document body. The motivation for this ties in with this part of what you wrote:

z-index is hierarchical. So an element can create a stacking context, which becomes the root for z-index values of its descendants.

Practically this means that an upper-bound value is created, for example:

<div style="z-index=2">I end up on top</div>
<div style="z-index=1">
    <div style="z-index=3">I don't end up on top</div>
</div>

I think that in an application we don't want this behaviour: if we render an Alert from some other layer, it should end up on top.

So apart from that point, everything is fully 'native'. And if it turns out that what we actually want is going completely native, would only be a matter of changing two lines of code. That's all.

@ismay
Copy link
Member

ismay commented Apr 16, 2020

I will dive into the details tomorrow. For now I'd like to reassure you that this current implementation stays very close to 'native' z-index behaviour.

Thanks, yeah I also still have to properly digest the info and actually dive into how it applies to our situation, etc. I'm quite interested in what Uber are doing, it sounded interesting. Feel free to minimize my original comment if it clutters up the PR too much btw.

And yeah, I guess we all want to stay close to the native situation. Consider me reassured 😊. It does seem like a bit of a tricky problem to solve, judging from all the articles and slightly different approaches out there. But anyway, I'm confident we'll end up with something proper 👍.

I'll see if I can digest the information above and come up with any thoughts on the matter.

@HendrikThePendric
Copy link
Contributor Author

HendrikThePendric commented Apr 16, 2020

I'll see if I can digest the information above and come up with any thoughts on the matter.

Thanks! And if you do please relate these thoughts to the current implementation.

Really everything that makes our solution divert from 100% native in these lines:

const LayerContext = createContext({
node: document.body,
level: 0,
})

const parentLayer = useContext(LayerContext)
const [layerEl, setLayerEl] = useState(null)
const nextLayer = {
node: layerEl,
level: Math.max(parentLayer.level, level),
}
const portalNode =
level > parentLayer.level ? document.body : parentLayer.node

  • The initial parent is the document.body at level 0
  • A new layer is appended to that
  • Any layer being stacked on top of that one checks if it has a higher level than the parent
    • If YES it gets appended to the body again to prevent the parent layer from limiting the z-index (see the HTML example in my previous comment for illustration)
    • If NOT it gets appended to the parent layer so we leverage 'native' stacking

@HendrikThePendric
Copy link
Contributor Author

I've gone through the articles. The different factors that were mentioned there were also taken into account when I worked on our implementation. I have come up with a similar but slightly different solution from Uber, and wouldn't know if it's any better or worse.

What I do know is the following:

  • The current implementation is an improvement over the previous one
  • The current implementation is simple and very close to 'native'.
  • If we want to be fully native, we can change this current implementation to be like that with some very minor (non-breaking) changes

@ismay let me know if you have any specific questions about, or have identified any issues with our current implementation.

@ismay
Copy link
Member

ismay commented Apr 16, 2020

So I did some initial testing, and the way I interpret Uber's approach is a bit different. The way I see it it's similar to what @Mohammer5 and I suggested initially. Though Uber went even further and prefers to completely omit z-indexes unless absolutely necessary.

So what they suggest is the following:

<body>
  <div id="app">All the app content here</div>
  <div id="overlay">Everything that needs to overlay the app goes here</div>
</body>

Their Layer component is then used to render content in #overlay. Positioning (absolute, fixed, etc.) then takes care of where to position it. But there's no z-index anywhere. The rendering order is what decides the visual order. So anything rendered in a <Layer /> will always overlap the app.

I experimented a bit with that approach, and I think it works quite well if you stick to their suggestions https://jsfiddle.net/eyxj8hfm/8:

Screenshot 2020-04-16 at 12 49 52

So components like the select could then use relative positioning for their dropdown (like it did initially). And they still break out of their containers as they should. It does seem like a nice approach to me.

This does mean that we need to adjust our current approach in areas. Plus it invalidates the stacking strategy from our design system. But if the net result is simplicity and less maintenance that could be worth it. I wonder, which use cases would not be covered by the above? I'd like to see what the shortcomings are (plus it's just an initial, superficial interpretation, I could be misinterpreting their suggestions).

edit:

So the way I interpret it combining and summarizing all the above recommendations:

  1. Omit z-index whereever possible
  2. Use an app layer, and an overlay layer
  3. In the app and overlay layers, dom order decides paint order, we can guarantee this by scoping the app and overlay containers at the root, so that developers (or 3rd party libs) can't accidentally break out of them
  4. If you absolutely have to use z-index within a component, scope it at the root with z-index: 0 and position: relative
  5. If we have to, we could provide more adjacent overlay layers at the root, where their ordering would just be decided by the dom order (but I wonder if that's necessary). That would allow for <Layer type="overlay" />, <Layer type="alert" />, or some other kind of semantic differentiation of stacking order.:
<body>
  <div id="app"></div>
  <div id="overlay">Modals and things go here</div>
  <div id="alert">Critical alerts (for example) go here, always rendering on top</div>
</body>

But I doubt whether we should.

@ismay
Copy link
Member

ismay commented Apr 16, 2020

So to respond a bit more directly to your question here:

@ismay let me know if you have any specific questions about, or have identified any issues with our current implementation.

So the behaviour you're describing here:

If a layer with a higher z-index is is created from a an existing layer with a lower z-index, than that layer will not be appended to the parent, but to the document body. The motivation for this ties in with this part of what you wrote:

z-index is hierarchical. So an element can create a stacking context, which becomes the root for z-index values of its descendants.

Practically this means that an upper-bound value is created, for example:

<div style="z-index=2">I end up on top</div>
<div style="z-index=1">
    <div style="z-index=3">I don't end up on top</div>
</div>

I think that in an application we don't want this behaviour: if we render an Alert from some other >layer, it should end up on top.

The way I interpret that is that you're making sure that all z-indexes apply to the same context. If there is a chance of scoping, you're breaking it out of it's container, to guarantee that the z-index value doesn't get scoped.

I think that we differ in opinion here. So with this approach, all z-index values operate in the same context. That is a simplification, compared to having multiple contexts. Which is nice, I understand the intention there (if I'm not mistaken).

But the concern I have with it is that it does require an awareness of all the other z-index values.

To me, the ideal in a component lib, is if when working on a component, you can just concern yourself with the component, without having to worry about any global effects. The thing that uber, and the articles are pointing towards in my interpretation, is a more semantic approach, where instead of providing the z-index value directly, you state the intent by rendering it in a <Layer> (with an optional type, if we need further ordering). That decouples everything a lot more in my opinion, which is what I'd personally prefer.

(I can imagine the comment above this one is probably still missing some edge-cases etc. It's meant more as an example of the direction I'm thinking in, as opposed to something complete.)

@varl
Copy link
Contributor

varl commented Apr 16, 2020

The "no z-index" solution is a very interesting technique that we should explore. If it works it sidesteps a whole class of bugs that can arise from z-index manipulation. If we can do so, we can also simplify the "layer stacking" aspects of the design system too.

@HendrikThePendric
Copy link
Contributor Author

HendrikThePendric commented Apr 16, 2020

In response to @ismay:

The way I interpret that is that you're making sure that all z-indexes apply to the same context.

Are you referring to a React context here or a z-index stacking context? For both these cases your interpretation seems incorrect:

  • In terms of React context: each layer produces a new one, it's a recursive mechanism
  • In terms of z-index context: nested elements with equal or lower z-index than their parent will be part of the same z-index stacking context. Same goes for siblings by the way.

If there is a chance of scoping, you're breaking it out of it's container, to guarantee that the z-index value doesn't get scoped.

The part I've made bold is not exactly how it works... I'm not making any assumptions here. The component only "broken out of its container" when we've established that the child layer will actually get scoped (as in gets a z-index upper-bound value imposed) by its parent.

But the concern I have with it is that it does require an awareness of all the other z-index values.

I think you'll have to explain what you mean by this in a little more detail. You might be right, in the sense that we are using making use of react context here, and the current implementation does sort of keep track of the layer hierarchy to an extend.... But I get the impression that might not be what you are referring to....

To me, the ideal in a component lib, is if when working on a component, you can just concern yourself with the component, without having to worry about any global effects.

I agree with that 100%. But I'm failing to see in what way the current implementation is failing to deliver on this. Some examples:

  • We can nest a MultiSelect in a Modal and the MultiSelect will come on top
  • In this PR I have refactored various components to use the Layer component. As far as I can tell, none of these implementations are "aware of any global effects"
  • Actually the whole idea of the current implementation is that the dev would just have a very simple notion of "layers with levels" and stacking those will just work in a uniform manner, independently of wherever a component lives in the component-tree...

The thing that uber, and the articles are pointing towards in my interpretation, is a more semantic approach, where instead of providing the z-index value directly, you state the intent by rendering it in a (with an optional type). That decouples everything a lot more in my opinion, which is what I'd personally prefer.

Could you explain your preferred approach by using some pseudo code examples? I'm not sure whether I understand what you are suggesting correctly.

I think I get the gist of it, but I am mainly failing to see how components could implement layers internally and it would work correctly when nesting etc. We have quite few components that have some form of anchor-element combined with some form of pop-up element, and I don't get how we'd make those work correctly (preferably out-of-the-box)....

@HendrikThePendric
Copy link
Contributor Author

I've made a tiny edit to the jsfiddle @ismay made to illustrate what I see as a problem https://jsfiddle.net/hendrikdegraaf/h9L1swjc/4/

@HendrikThePendric
Copy link
Contributor Author

HendrikThePendric commented Apr 16, 2020

The "no z-index" solution is a very interesting technique that we should explore. If it works it sidesteps a whole class of bugs that can arise from z-index manipulation. If we can do so, we can also simplify the "layer stacking" aspects of the design system too.

We can change the current solution to not use any z-index values too:

  • Use several root elements to append portals too, one root per whatever level is in the design system
  • DOM order would dictate overlap, i.e. the alert root element could be the last one in the DOM so it's always on top of the others
  • We keep nesting layer elements as we do now so a nested Select will still end up on top of of its parent Modal

Disclaimer:

  • We don't eliminate any complexity
  • We make things less flexible (we'd have to stick to whatever layers are in the design system), because we need a root element for each one.
  • Things would be less React friendly because we'd be appending these root elements to the DOM in plain JS.

bugs that can arise from z-index manipulation

Just to be clear: the current implementation does zero z-index manipulation. All it does is append some layers to the root, i.e. document.body, and others to the parent layer. So there is some manipulation being done, but that's in terms of DOM nesting, not z-index.

@HendrikThePendric
Copy link
Contributor Author

As a more general note:
I'm noticing that I am "defending" my current implementation, because I believe it's pretty simple and the behaviour it provides is convenient and correct. I really appreciate all the research that has been done and the feedback I've received, but I don't think we can compare an actual implementation with an outline for a general approach. So I guess the only logical next step would be to see a (basic) implementation of an alternative approach. Then we can start comparing apples to apples.

@varl
Copy link
Contributor

varl commented Apr 16, 2020

Thinking on this some more, this is going into the alpha version, so while we figure out if there is a viable alternative approach, we can merge this and see how it works for us right now.

That way @ismay doesn't have to drop what he's doing and scramble to get something put together right now, and this PR would not have to stay open until we have a "perfect" solution, and @HendrikThePendric can proceed.

The argument against merging this is if changing it in the future is so hard, that we are for all practical purposes locked into this approach. It doesn't seem that way to me though.

@HendrikThePendric
Copy link
Contributor Author

It doesn't seem that way to me though.

Agreed 👍

Copy link
Contributor

@varl varl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be easier to change in the future if we want to implement other logic for the internals. It improves the E2E test cases and fixes some other bugs as well. I think we can go ahead and merge this and pull out the discussion about if there are other Layer handling strategies to an issue as @ismay has written down some very valuable information that would be a shame to hide in a PR.

Changes the set of components used to produce various types of overlays:
- `Layer` is an overlay component that fills the entire screen/page. Besides that it is also a key component to stack various components on top of one another.
- `ComponentCover` is a similar component that only fills its parent, provided that has a non-static position.
- Both the `Layer` and the `ComponentCover` can be blocking or non-blocking, accept an `onClick` and a `translucent` prop.
- `CenterContent` is a component that does exactly what it says on the tin. It also has a `position` prop which can be used to vertically align the content at the `top`, `middle` (default), or `bottom`.

BREAKING CHANGE:
These new components replace the `Backdrop` and the `ScreenCover`, which had a slightly unclear scope and have now been removed.
@HendrikThePendric HendrikThePendric force-pushed the refactor/reorganise-layers-and-overlays branch from b25db4b to 24ead4c Compare April 16, 2020 13:50
@ismay
Copy link
Member

ismay commented Apr 16, 2020

Agreed, that sounds practical 👍. Would be a shame to block v5. By the way @HendrikThePendric, my intention was just to discuss the different approaches, apologies if I came across as attacking the PR, nothing personal 😊.

@HendrikThePendric
Copy link
Contributor Author

By the way @HendrikThePendric, my intention was just to discuss the different approaches, apologies if I came across as attacking the PR, nothing personal 😊.

No worries @ismay I realise that you were doing just that, and I think all the input you provided was very valuable. It could well be that it leads to an even better implementation in the future.

By the way:

I think we can go ahead and merge this and pull out the discussion about if there are other Layer handling strategies to an issue as @ismay has written down some very valuable information that would be a shame to hide in a PR.

Could you move your findings somewhere @ismay ?

@ismay
Copy link
Member

ismay commented Apr 16, 2020

Could you move your findings somewhere @ismay ?

Will do 👍

edit: issue is here #25

@HendrikThePendric HendrikThePendric merged commit 5a80f71 into alpha Apr 16, 2020
@dhis2-bot
Copy link
Contributor

@HendrikThePendric HendrikThePendric deleted the refactor/reorganise-layers-and-overlays branch April 16, 2020 14:10
@HendrikThePendric HendrikThePendric mentioned this pull request Apr 25, 2020
@dhis2-bot
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this will break the existing API released on @alpha released
Development

Successfully merging this pull request may close these issues.

None yet

4 participants