fix(Button): default opacity for disabled clear buttons

[KillerCodeMonkey]

Clear buttons to not get disabled state, when attribute disabled is set.

<ion-header>
  <ion-toolbar>
    <ion-title>
      Ionic Blank
    </ion-title>
  </ion-toolbar>
</ion-header>

<ion-content>
  <div class="ion-padding">
    The world is your oyster.
    <p>If you get lost, the <a target="_blank" rel="noopener" href="https://ionicframework.com/docs/">docs</a> will be your guide.</p>
  </div>
</ion-content>

<ion-footer>
  <ion-toolbar>
    <ion-buttons>
      <ion-button fill="clear" color="primary" disabled>clear disabled</ion-button>
      <ion-button fill="clear" color="primary">clear enabled</ion-button>
      <ion-button fill="solid" color="primary" disabled>solid disabled</ion-button>
      <ion-button fill="solid" color="primary">solid enabled</ion-button>
      <ion-button fill="outline" color="primary" disabled>outline disabled</ion-button>
      <ion-button fill="outline" color="primary">outline enabled</ion-button>
    </ion-buttons>
  </ion-toolbar>
</ion-footer>

as you can see the clear buttons do not have the default disabled opacity set ;).

In ionic v4.4:

#18509 (comment)

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Since 4.5.0 the opacity: .5; does not work for clear buttons.

Issue Number:
introduced with PR #18509
#18555

What is the new behavior?

opacity is set to the default opacity: .5; ion-buttons with the fill="clear" and disabled attributes.

Does this introduce a breaking change?

• Yes
• No

Other information

• works in v4.4

[brandyscarney]

Thank you for the PR! Unfortunately this fix would not allow for users to override the disabled opacity because it would not be inherited in the .button-native from the host element. I believe the fix for this issue is to remove the following line here: https://github.com/ionic-team/ionic/blob/master/core/src/components/button/button.md.scss#L63

[brandyscarney]

Please see my previous comment. 🙂

[KillerCodeMonkey]

ah okay. i only checked the changes, which introduced the problem. I am on it

[KillerCodeMonkey]

@brandyscarney i think the $button-md-clear-opacity scss var is obsolete then?

it is only used in the md variables.scss where it is defined

[KillerCodeMonkey]

@brandyscarney is there a trick to get yarn start working? After merging the master it fails with

Error: invalid outputTarget type "experimental-dist-module". Valid target types: angular, dist, docs, docs-json, docs-custom, stats, www at config.outputTargets.forEach.outputTarget

[brandyscarney]

@KillerCodeMonkey I am not sure, I only ever use npm start. Yes it looks like the scss var is obsolete.

[brandyscarney]

@KillerCodeMonkey Oh you may need to run a fresh npm i if you haven't lately. We just merged some big changes in with Stencil yesterday.

[KillerCodeMonkey]

But i Leave IT there If Somebody needs the Default value from the Framework (which could possibly Change) There are other unused vars in the buttons vars scss. PS: i Made a freshinstall an installed sass AS global dep. Brandy Carney <notifications@github.com> schrieb am Fr., 21. Juni 2019, 17:11:

…

@KillerCodeMonkey <https://github.com/KillerCodeMonkey> I am not sure, I only ever use npm start. Yes it looks like the scss var is obsolete. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18560>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARI4YARSYK6FCTSXT3EMI3P3TVRFANCNFSM4HYWERPA> .

[simonhaenisch]

Will this be merged or fixed soon? I thought you have screenshot diff tests to catch these kind of regressions?

BTW a workaround is to use fill="none" which is not valid and breaks the colors but other than that looks the same as "clear", and disabled opacity works (https://codepen.io/simonhaenisch/pen/KjWejq?editors=1000).

[KillerCodeMonkey]

@brandyscarney it should be fixed. i tested it and everything runs with success :).

for removing obsolete sass vars i would create a nother PR if your team would appreciate it.

[brandyscarney]

I thought you have screenshot diff tests to catch these kind of regressions?

We do, but the disabled button in the clear test is towards the bottom of the page and the way our screenshot tool is set up it is being cut off by the browser height. We need to move the button to a new test where it will be in the visible screenshot or increase the browser height, but then we run the risk of this happening again: https://screenshot.ionicframework.com/data/tests/34dfc3c/src/components/button/test/clear/

[simonhaenisch]

I see. Couldn't you utilize sth like scrollIntoView (or window.scrollTo(0, document.body.scrollHeight)) to get a different scroll position for a second screenshot and then do two screenshot diffs?

Anyway, not really related to this PR (: This will be fixed with the 4.6 release, right?