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

Anchors with ion-button and color are getting text and button color classes #8293

Closed
brandyscarney opened this issue Sep 29, 2016 · 10 comments · Fixed by #9096
Closed

Anchors with ion-button and color are getting text and button color classes #8293

brandyscarney opened this issue Sep 29, 2016 · 10 comments · Fixed by #9096
Assignees
Milestone

Comments

@brandyscarney
Copy link
Member

The following markup:

  <a ion-button color="secondary">Button</a>

results in a button that looks like this:

screen shot 2016-09-29 at 1 55 55 pm

caused by it getting both the .text-secondary (anchor) and .button-secondary classes.

@brandyscarney brandyscarney added this to the 2.0.0-rc.1 milestone Sep 29, 2016
@brandyscarney brandyscarney self-assigned this Sep 29, 2016
@brandyscarney
Copy link
Member Author

Didn't realize this was already created and fixed by #8249. Leaving open to add unit tests to avoid this happening in the future.

@alan-agius4
Copy link
Contributor

Same thing is happening on;

<a ion-item color="secondary">item</a>

@manucorporat
Copy link
Contributor

@alan-agius4 @brandyscarney maybe we should not provide [color] in <a> directly.

@alan-agius4
Copy link
Contributor

alan-agius4 commented Oct 2, 2016

@brandyscarney , @manucorporat

Personally, I am not a big fan of this directive, It sort of turn primitive HTML elements into angular components and the results can be achieved using CSS classes only or in the case of Ionic utility attributes.

ex for primitive elements rather than
<span color="primary"></span>

would be:
<span color-primary></span>

Reason being is that utility attributes would be more flexible since you can apply them on any element even those that are not listed within the directive. ex: time, article, summary etc... More over, it can be applied on your grid system which is missing from this directive. Example on an ion-col.

Also, to extanded that another utility attribute can be added for the background color. ex: background-primary or bg-primary.

Proposed code:

.ios {
  @each $color-name, $color-base, $color-contrast in get-colors($colors-ios) {

    [color-#{$color-name}] {
      color: $color-base;
    }

    [background-#{$color-name}] {
      color: $color-contrast;
      background-color: $color-base;
    }

  }
}

I can create a PR if you agree with my proposal.

@brandyscarney
Copy link
Member Author

Thanks for the feedback @alan-agius4! The issue is we have had several people wanting to add a color based on a data set or variable, so with an attribute you could add it based on some expression:

<a [attr.color-primary]="isMd ? '' : null">My Link</a>

However, if you don't know the color that you want is primary you can't add the attribute (easily). For example, if you were looping through items and each one had a myColor property that you wanted to use as the color which evaluated to primary or secondary. With our current syntax, you could write something like:

<a [color]="item.myColor">My Link</a>

I'm not sure of the best solution for this. Looping in @adamdbradley for discussion.

@alan-agius4
Copy link
Contributor

alan-agius4 commented Oct 3, 2016

Conviencer me there :) probably the neatest approach is what you suggested earlier, to add a unique selector for anchor tags.

Ex: <a ion-link color="primary">link</a>

Unless you want to consider adding Utility CSS Classes in your framework, class name can be text-primary, color-primary or simply primary

<a [ngClass]="'primary'">link</a> // string
<a [ngClass]="item.myColor">link</a> // in loop
<a [ngClass]="{'primary': isMd}">link</a>` //conditional

@brandyscarney
Copy link
Member Author

I'm thinking that may be the best solution. With a custom directive for anchor tags we wouldn't be automatically styling anchors to have the Ionic look. Adam and I are going to discuss this some more and I'll update the issue after. :)

@infinnie
Copy link

The same problem here.

@brandyscarney brandyscarney modified the milestones: 2.0.0-rc.1, 2.0.0-rc.2 Oct 13, 2016
@brandyscarney
Copy link
Member Author

Submitted a pull request for this here: #9096

Basically it adds an attribute called ion-text that should be added to any HTML element in order to use the color input. All of the other selectors are still there with a deprecation warning if you are not using the ion-text attribute and ion-item, ion-fab and ion-button are excluded from the a tag.

cc @alan-agius4 let me know what you think!

@alan-agius4
Copy link
Contributor

@brandyscarney, Its good :) much clean than the current

adamdbradley pushed a commit that referenced this issue Nov 10, 2016
…hers

* fix(fab): remove chained css and exclude from typography directive

references #8293

* refactor(typography): add `ion-text` attribute selector, deprecate others

closes #8293
@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

Successfully merging a pull request may close this issue.

5 participants