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

Add a z-index input property on ng-http-loader component #189

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tsepton
Copy link

@tsepton tsepton commented Dec 13, 2023

Add a way to specify a z-index for the component. Default stays 9999.

PS: thanks for this nice package.

@coveralls
Copy link

coveralls commented Dec 13, 2023

Pull Request Test Coverage Report for Build 7196877749

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7165466536: 0.0%
Covered Lines: 89
Relevant Lines: 89

💛 - Coveralls

@mpalourdio
Copy link
Owner

Thanks for this PR.
I'm wondering if we are just not overthinking things here, as it's really easy to do in plain CSS, no ?

@tsepton
Copy link
Author

tsepton commented Dec 13, 2023

This is the actual solution that I had inside my app. However, if the CSS selector changes overtime, the added CSS will fail. Also, maybe I am missing a cleaner way of doing it, but having to add .backdrop application wide (i.e. inside the style.scss of the angular app) may have side effects too

I don't know what the good practice is here

@mpalourdio
Copy link
Owner

mpalourdio commented Dec 13, 2023

Sure, that's why i'm wondering if we should not think about a more universal solution, like giving the ability to specify a custom CSS class, so people consuming this lib are safer, and have much more css possibilities.With a css class, the selector is directly handled by the dev, and overrides are much simpler

@tsepton
Copy link
Author

tsepton commented Dec 13, 2023

A much better solution! I'll make these changes asap

@mpalourdio
Copy link
Owner

Take your time to battle test it. And please, let's discuss in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants