-
Notifications
You must be signed in to change notification settings - Fork 174
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
Change per-component backdrop to global one #116
Change per-component backdrop to global one #116
Conversation
<TopNav pathData={navigationPath} /> | ||
<LeftNav pathData={navigationPath} /> | ||
</div> | ||
<Backdrop> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally wouldn't add the Backdrop element outside the application container. And I believe we don't need to wrap any element inside the Backdrop. I would write this markup like that:
<div id="app">
<Backdrop></Backdrop>
<div class="fd-page {hideNav? 'iframeContainerNoNav' : 'iframeContainer'}" use:init="context"></div>
<TopNav pathData={navigationPath} />
<LeftNav pathData={navigationPath} />
</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my approach is more natural and makes the code more referring to what we can actually see and do in the app. The backdrop wraps whole content and makes it unusable/unclickable, that's why I wrapped content
in backdrop
. I can change it if you really want, it's basically just code-understanding thing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about modifying your suggestion to
<div id="app">
<Backdrop>
<div class="fd-page {hideNav? 'iframeContainerNoNav' : 'iframeContainer'}" use:init="context"></div>
<TopNav pathData={navigationPath} />
<LeftNav pathData={navigationPath} />
</Backdrop>
</div>
?
aria-hidden="false" | ||
> | ||
</div> | ||
<div class="fd-ui__overlay fd-overlay fd-overlay--modal {heightCssClass}" aria-hidden="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these slot tags here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we wrap App's content in <Backdrop>
tags. <slot>
is for displaying what's inside component tags. It's the same as <ng-content>
in Angular. Am I wrong?
core/src/App.html
Outdated
@@ -220,6 +218,10 @@ | |||
cursor: pointer; | |||
} | |||
|
|||
.fd-page{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is the right way to do it, otherwise it will be inconsistent, and we will continue having displaying issues. But I am not convinced with the fact that we are providing a feature but it does not work out of the box, since every app using luigi needs to wrap the iframe in a container styled with this z-index value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This z-index value is assigned at Lugii Core's level so it works out of the box. The app developer doesn't have to implement any z-index value to the container.
ec92d42
to
94396ed
Compare
* Change per-component backdrop to global one * Change Backdrop to wrap App's content instead of App
Description
Instead of having separate backdrop for
LeftNav
andTopNav
components we now cover the wholeApp
component with backdrop which is easier to understand and more natural according to what the backdrop actually does. The<iframe>
with page content is displayed above the backdrop layer so it has to have its own backdrop (no changes here).Changes proposed in this pull request:
Related issue(s)
#108