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

bug: no-padding / no-margin not working on toolbar #8765

Closed
brandyscarney opened this issue Oct 17, 2016 · 10 comments
Closed

bug: no-padding / no-margin not working on toolbar #8765

brandyscarney opened this issue Oct 17, 2016 · 10 comments
Assignees
Milestone

Comments

@brandyscarney
Copy link
Member

If the attribute is added to a toolbar the padding on the toolbar takes precedence:

<ion-toolbar no-padding>

A simple solution to this is moving the styles to the end of the css so they take precedence over toolbar, but this could always get messed up again in the future, and it probably doesn't work on some components.

This is just a thought, but what if we add a utility mixin that automatically added all of these attributes to the main component class. So it would be like:

.toolbar-md {
  @include util();

  // all the other toolbar stuff
}

and then the mixin could be:

@mixin util() {
  &[no-padding] {
    padding: 0;
  }

  // all of the other padding attributes

  &[no-margin] {
    margin: 0;
  }

  // all of the other margin attributes

  // any other util attributes
}

None of the above was tested so it could not be functional currently.

What do you think @manucorporat @adamdbradley @bensperry ?

@tlaverdure
Copy link
Contributor

This also effects other areas like:

<ion-card no-margin>

Which has no effect once rendered because .list-ios overrides no-margin.

<ion-card no-margin class="list-ios">
margin: -1px 0 32px 0;

Maybe these utilities should use !important?

@brandyscarney
Copy link
Member Author

@tlaverdure Yeah I was trying to avoid using !important but I suppose this is one of those cases where you wouldn't add the attribute if you didn't want it to always take precedence.

@tlaverdure
Copy link
Contributor

@brandyscarney that is exactly what I was thinking, since it's implicitly declared it shouldn't hurt.

@j3gb3rt
Copy link

j3gb3rt commented Nov 2, 2016

@brandyscarney To avoid the use of important, would it be possible to just extend a base component and go back to the way styles were applied in beta 11. You could then have something like ion-card-md which would always have the md styles applied to it. I don't think this would add much to package size or complexity of the frameworks css. It would make overriding easy again and would mean I don't have to rewrite at lot of the selectors I had written for beta 11 😝

@j3gb3rt
Copy link

j3gb3rt commented Nov 2, 2016

I found another solution. If all of the utility variables are places inside .ion-page it causes there selectors to have higher priority.

.ion-page {
  [no-margin] {
    margin: 0;
  }
}

This also works on the page level

example-page.ion-page {
  [ion-card] {
     background-color: #000;
  }
}

I will use the for now, but it does some extra work and isn't very apparent. If possible it would be nice to have a system that makes it easier to override styles without having to add an id or class to every element of a certain type (like ion-card)

@j3gb3rt
Copy link

j3gb3rt commented Nov 2, 2016

Another possible quick fix for the utilities is to rearrange the current selectors.
"no-margin" is current
[no-margin], [no-margin] .scroll-content {

Moving .scroll-content before the [no-margin] fixes the priority as well
[no-margin], .scroll-content [no-margin] {

I would put up a pull request for this, but unfortunately my company doesn't let me contribute to open source. Great framework though. It's made our app development so fast!

@brandyscarney
Copy link
Member Author

Thanks for the ideas @j3gb3rt! We're going to be discussing this more in depth and try to come up with a solution that works for everyone. 😄

@brandyscarney brandyscarney added this to the 2.0.0-rc.3 milestone Nov 4, 2016
@biesbjerg
Copy link

While adding !important to utility classes will work for those utilities, it doesn't solve other styling use-cases:

In this example my custom styling is overridden by the platform specific style:

login-form {
    ion-list {
        margin: 0;
    }
}

image

@brandyscarney brandyscarney modified the milestones: 2.0.0-rc.3, 2.0.0-rc.4 Nov 17, 2016
@ncapito
Copy link
Contributor

ncapito commented Dec 2, 2016

@biesbjerg you could fix your issue by being more specific.

e.g. change login-form to .login-form

.login-form {
    ion-list {
        margin: 0;
    }
}

@brandyscarney
Copy link
Member Author

So we decided to go with a solution similar to the one proposed by @j3gb3rt. I've prefixed the utility attributes with ion-app and the mode (in certain cases). This makes it possible to change the padding/margin per mode again since that wasn't working. For example, you will be able to use:

$content-ios-padding: 16px;
$content-md-padding: 24px;
$content-wp-padding: 10px;

and that will set the default padding added by using padding-top, padding-bottom, etc. to a different value based on the mode. I didn't use !important because this caused conflicts with the status bar padding that gets added for iOS.

@biesbjerg Your issue doesn't seem to be caused by the utility attributes since you are setting the padding in the CSS directly (please correct me if I'm misunderstanding). The components now have a class added without the mode attached that can be used to style the component for all modes. So you could do something like this:

login-form .list {
  margin: 0;
}

This will be in the rc.4 release and I have released a nightly if you'd like to check it sooner. Please let me know if you find any issues:

npm install --save ionic-angular@nightly

Thanks! :)

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants