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: Buttons not getting the correct class #18085

Closed
jwargo opened this issue Apr 19, 2019 · 14 comments

Comments

5 participants
@jwargo
Copy link

commented Apr 19, 2019

Bug Report

Ionic version:

[x] 4.x

Current behavior:

this code:

 <ion-button slot="start">
            <ion-icon name="arrow-dropup" slot="icon-only"></ion-icon>
          </ion-button>
          <ion-label text-wrap (click)="buttonModify(button)"> {{button.title}} </ion-label>
          <ion-button slot="end">
            <ion-icon name="arrow-dropdown" slot="icon-only"></ion-icon>
          </ion-button>

generates the following output. The buttons don't size the same.

image

Expected behavior:

The buttons would be the same size.

Steps to reproduce:

From my conversation on Slack with Mike Hartington:

ou use the slots to place content on the left/right of the item
27 replies

johnwargo   [39 minutes ago]
yeah, that's where I started and it didn't work right either (edited)

johnwargo   [39 minutes ago]
i'll go back and try that again and let you know. Thanks!!

mike (do not ping)   [38 minutes ago]
did you add the slots for the icons/buttons?

mike (do not ping)   [38 minutes ago]
<ion-button slot="start">
            <ion-icon name="arrow-dropup" slot="icon-only"></ion-icon>
          </ion-button>
          <ion-label text-wrap (click)="buttonModify(button)"> {{button.title}} </ion-label>
          <ion-button slot="end">
            <ion-icon name="arrow-dropdown" slot="icon-only"></ion-icon>
          </ion-button>

mike (do not ping)   [38 minutes ago]
basically, the slot="icon-only" will size thing consistently

johnwargo   [38 minutes ago]
OK, thanks.

johnwargo   [37 minutes ago]
although I didn't try `icon-only`.

johnwargo   [30 minutes ago]
No change (except the first one is pushed left)
This file was deleted.

mike (do not ping)   [29 minutes ago]
Mmm, looks like something else is causing this

mike (do not ping)   [28 minutes ago]
any chance you could push this to github?

johnwargo   [28 minutes ago]
its already there. what's our github ID? I'll add you to it

mike (do not ping)   [27 minutes ago]
mhartington

johnwargo   [25 minutes ago]
https://github.com/johnwargo/timeslicer-ionic

johnwargo   [24 minutes ago]
sorry, this branch: https://github.com/johnwargo/timeslicer-ionic/tree/new-project-form

johnwargo   [24 minutes ago]
project-edit page

mike (do not ping)   [16 minutes ago]
ios or android?

johnwargo   [12 minutes ago]
browser

johnwargo   [12 minutes ago]
ionic serve

johnwargo   [11 minutes ago]
haven't tried it on a phone yet (or emulator/simulator)

johnwargo   [10 minutes ago]
Oh, and I did the grid so the first one (with no up button) would line up correctly with the other rows.

mike (do not ping)   [6 minutes ago]
Hmm, still seems not right. could you open an issue for this?

johnwargo   [5 minutes ago]
Sure

johnwargo   [4 minutes ago]
where? Main repo? (edited)

mike (do not ping)   [3 minutes ago]
yeah

mike (do not ping)   [3 minutes ago]
http://github.com/ionic-team/ionic
GitHub
ionic-team/ionic
Build amazing native and progressive web apps with open web technologies. One app running on everything :tada: - ionic-team/ionic

mike (do not ping)   [3 minutes ago]
I know what the issue is

mike (do not ping)   [3 minutes ago]
the buttons are not getting the right classes they need

Related code:

 <ion-button slot="start">
            <ion-icon name="arrow-dropup" slot="icon-only"></ion-icon>
          </ion-button>
          <ion-label text-wrap (click)="buttonModify(button)"> {{button.title}} </ion-label>
          <ion-button slot="end">
            <ion-icon name="arrow-dropdown" slot="icon-only"></ion-icon>
          </ion-button>
insert short code snippets here

Other information:

Ionic info:

insert the output from ionic info here

Ionic:

ionic (Ionic CLI) : 4.12.0 (/Users/johnwargo/.nvm/versions/node/v11.12.0/lib/node_modules/ionic)
Ionic Framework : @ionic/angular 4.0.1
@angular-devkit/build-angular : 0.12.4
@angular-devkit/schematics : 7.2.4
@angular/cli : 7.2.4
@ionic/angular-toolkit : 1.3.0

Capacitor:

capacitor (Capacitor CLI) : 1.0.0-beta.19
@capacitor/core : 1.0.0-beta.17

System:

NodeJS : v11.12.0 (/Users/johnwargo/.nvm/versions/node/v11.12.0/bin/node)
npm : 6.9.0
OS : macOS Mojave

@ionitron-bot ionitron-bot bot added the triage label Apr 19, 2019

@jwargo

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

@mhartington created this codepen to help me troubleshoot this: https://codepen.io/mhartington/pen/xejmOL?editors=1000

@johnwargo

This comment has been minimized.

Copy link

commented Apr 20, 2019

I played around with this today and I just noticed that the class gets set correctly if I open the page again. So it's only incorrect when I add an item to the list.

@brandyscarney

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Thanks for the issue! Could you update the code or provide an example of it broken? The code in the issue & the Codepen Mike created show the buttons looking correct.

Thanks!

@ionitron-bot ionitron-bot bot removed the triage label May 6, 2019

@jwargo

This comment has been minimized.

Copy link
Author

commented May 7, 2019

@brandyscarney sorry, I don't. I pasted in my code above when I encountered the issue Mike asked me to make the issue. I've since moved on to a different approach (which works quite well). Mike has access to the repo although I've deleted the branch.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply labels May 7, 2019

@mhartington

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Example code is

    <ion-item *ngFor="let button of project.buttons; let idx= index" no-padding>
      <ion-button *ngIf="idx>0" (click)="buttonUp(idx)" slot="start">
        <ion-icon name="arrow-dropup" slot="icon-only"></ion-icon>
      </ion-button>
      <ion-label text-wrap (click)="buttonModify(button)"> {{button.title}} </ion-label>
      <ion-button *ngIf="idx<project.buttons?.length-1" (click)="buttonDown(idx)" slot="end">
        <ion-icon name="arrow-dropdown" slot="icon-only"></ion-icon>
      </ion-button>
    </ion-item>

TL;DR, ngIf on the ion-button will prevent the size="small" attribute being set. Looks like the host data isn't being set and the css/size is not being applied.

@brandyscarney

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Thanks Mike! This is likely the problem code in item:

  componentDidLoad() {
    // Change the button size to small for each ion-button in the item
    // unless the size is explicitly set
    Array.from(this.el.querySelectorAll('ion-button')).forEach(button => {
      if (button.size === undefined) {
        button.size = 'small';
      }
    });

We should move this to check on the button if it is in an item, or possibly move it to CSS.

@ionitron-bot

This comment has been minimized.

Copy link

commented May 7, 2019

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

@topalavlad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

I'd like to take a look at this issue but I'm a bit confused about how to write a test containing *ngFor since all other tests are very basic. I tried accessing fields declared in <script> from the HTML file but although methods are working, fields aren't. I'm guessing they should be wrapped in a class somehow.
Is there any recommended way to go about debugging this issue?

@brandyscarney

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Thanks @topalavlad! Our tests are written in vanilla JavaScript, so basically you wouldn't use Angular to reproduce it in an e2e test.

Since the code is happening after the item loads, I think the *ngIf on the button may be causing the issue. I created the following Codepen to reproduce this, you have to click "Add a Button" to see: https://codepen.io/brandyscarney/pen/VOMaXw?editors=1111

@topalavlad topalavlad referenced this issue May 27, 2019

Merged

fix(button): new button has incorrect size #18395

6 of 13 tasks complete
@topalavlad

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

We should move this to check on the button if it is in an item, or possibly move it to CSS.

I tried doing it via css but unfortunately didn't have much luck. The descendant selector doesn't seem to work. Just for testing, I removed ion-item from the selector below and all ion-button components became small so the descendant selector seems to be the problem:

:host(ion-item ion-button:not(.button-small):not(.button-default):not(.button-large)) {
  @extend .button-small;
}

Any suggestions?

@brandyscarney

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@topalavlad It might not be possible in just CSS at the moment. I was thinking of using hostContext in order to set the size on the button, like the following line:

'in-item': hostContext('ion-item', el)

https://github.com/ionic-team/ionic/blob/master/core/src/components/checkbox/checkbox.tsx#L141

The problem is the size would need to update if the user sets it on the button. By default it would be small if inside of an item, unless the user set it.

This could probably just be done in the componentWillLoad of button.tsx instead like the check for in a toolbar:

  componentWillLoad() {
    this.inToolbar = !!this.el.closest('ion-buttons');
    this.inItem = !!this.el.closest('ion-item');
  }

Then in hostData (this is untested code):

    let size = this.size;
    if (size === undefined && this.inItem) {
      size = 'small';
    }
@topalavlad

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Is doing it in hostData better/cleaner than directly in componentWillLoad like in the PR?

brandyscarney added a commit to topalavlad/ionic that referenced this issue Jun 10, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@topalavlad So the reason hostData is better is because it will work with the size dynamically changing. In componentWillLoad it will only set the size when the button initializes, but every time you change the size it will check for item. So if you start with the following:

<ion-item>
  <ion-button slot="start" size="large">Button</ion-button>
</ion-item>

And then remove the size, it will still update to use small. Your PR looks great though! I pushed a small change for this.

@brandyscarney brandyscarney added this to Backlog 🤖 in Ionic Core via automation Jun 10, 2019

brandyscarney added a commit that referenced this issue Jun 10, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Thanks for the issue and @topalavlad for the PR! I accidentally added an extra number sign in the merge, but this is fixed by a3e23fc. 😁

Ionic Core automation moved this from Backlog 🤖 to Done 🎉 Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.