Skip to content

chore(layer): demo issue with context from different modules#16

Closed
HendrikThePendric wants to merge 2 commits intoalphafrom
reproduce-layer-issue
Closed

chore(layer): demo issue with context from different modules#16
HendrikThePendric wants to merge 2 commits intoalphafrom
reproduce-layer-issue

Conversation

@HendrikThePendric
Copy link
Copy Markdown
Contributor

This is not a real PR, it's just a minimalistic demo of the issue @martinkrulltott found....

We should look into a way of creating the context differently....

@HendrikThePendric HendrikThePendric requested review from a user, Mohammer5 and varl March 31, 2020 15:09
@HendrikThePendric HendrikThePendric self-assigned this Mar 31, 2020
zIndex: propTypes.number,
}

export { LayerTwo, useLayer }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just a duplicate of the Layer from LayerContext.js but with a different name.

Comment on lines +22 to +33
export const ItFallsApart = () => (
<Layer zIndex={FIXED_INDEX}>
{zIndexComputed => (
<>
<h1>{zIndexComputed}</h1>
<LayerTwo zIndex={FIXED_INDEX}>
{anotherZIndexComputed => <h1>{anotherZIndexComputed}</h1>}
</LayerTwo>
</>
)}
</Layer>
)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It goes wrong here, because each layer is referring to a different base context.

@HendrikThePendric
Copy link
Copy Markdown
Contributor Author

In 1e5c903 I've found a "fix" for this problem. The only way I could think of was to attach the context to the window to avoid duplicates from being created. It's definitely not the most elegant solution available, but I couldn't think of another way to solve it. Any other suggestions would be most appreciated....

Personally I can't think of any other way to solve this, apart from not allowing several instances of ui-core from being used in one app.... Which, to be fair, probably is something worth discussing further anyway....

@ghost
Copy link
Copy Markdown

ghost commented Apr 1, 2020

Are we looking for an intermediate solution here, while we figure out a long term solution to the whole zindex issue? Just for some context on the solution we're looking for, because as an intermediary solution something dirty seems less problematic.

@HendrikThePendric
Copy link
Copy Markdown
Contributor Author

This pull request was just there to illustrate #15 which is now closed, so we can close this PR too.

For the record: in the end we decided not to fix this in our library, but to let the app prevent this problem from occurring, see #17.

@HendrikThePendric HendrikThePendric deleted the reproduce-layer-issue branch April 1, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant