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

feat(toast): add header and custom toast buttons #17147

Merged
merged 41 commits into from Apr 11, 2019

Conversation

zakton5
Copy link
Contributor

@zakton5 zakton5 commented Jan 17, 2019

Short description of what this resolves:

Allows more complex toasts to be utilized by allowing a header and custom buttons as discussed here: #16791

With this new feature, is showCloseButton and closeButtonText needed? I would opt for removing these properties entirely in favor of

const toast = await this.toastController.create({
      header: 'Toast header',
      message: 'Click to Close',
      position: 'top',
      buttons: [
        {
          side: 'end',
          text: 'Close',
          role: 'cancel'
        }
      ]
    });

Changes proposed in this pull request:

  • Add header property to toast options
  • Add buttons property to toast options

Ionic Version:

Fixes: #16791, possibly #16268, #16237

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Jan 17, 2019
@zakton5 zakton5 changed the title Feat toast actions feat(toast): add header and custom toast buttons Jan 17, 2019
@paulstelzer
Copy link
Contributor

Thanks a lot :) I have not checked it yet, but seems good!

@simonhaenisch
Copy link
Contributor

With this new feature, is showCloseButton and closeButtonText needed? I would opt for removing these properties entirely

Now that v4 is officially out, I guess you should keep them, transform and push into the buttons array if set, and add a note that they are deprecated. Introducing a breaking change might not be the best choice at the moment. The props can be dropped once a v5 release is close I guess.

@zakton5
Copy link
Contributor Author

zakton5 commented Feb 16, 2019

@simonhaenisch @brandyscarney I've implemented your suggestion. Let me know what you think.

@zakton5
Copy link
Contributor Author

zakton5 commented Mar 28, 2019

@liamdebeasi I made those changes. Let me know if anything else needs to change.

@brandyscarney
Copy link
Member

This is so great! Sorry it took me so long to look at this. I made some minor changes:

  • Remove the classes that only added padding to the content based on the presence of buttons - this caused a bug where there was no padding on toast messages without buttons
  • Default the side to "end" if one isn't provided
  • Added a buttons test showing the old way (close props), new way (buttons array), and multiple button examples
  • Updated the usage examples (some were still using slot, react didn't have usage for buttons)
  • Adds the button role class to the button, so if the button is a cancel button it will default to the lighter (almost white) color

Unless there are any complaints, this looks good to merge to me. @liamdebeasi did you want to take another look at this?

Here are some screenshots of latest:

Two Buttons Multiple Buttons
localhost_3333_src_components_toast_test_buttons(Galaxy S5) (2) localhost_3333_src_components_toast_test_buttons(Galaxy S5) (3)
localhost_3333_src_components_toast_test_buttons(iPhone 6_7_8 Plus) (1) localhost_3333_src_components_toast_test_buttons(iPhone 6_7_8 Plus) (2)

Copy link
Member

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 🎉

- adds a background color that is a transparent layer of the text color in md
- adds opacity to the button in ios
@brandyscarney
Copy link
Member

Okay pushed a few more changes:

  • Added a background color to the button in MD on hover (this is a transparent version of the text color)
  • Added opacity to the buttons in iOS on hover
  • Added margin end for MD buttons on the end side, and margin start for the start side

The only other thing I can find is the icon-only buttons should be rounded on hover/ripple but it could possibly be pushed to a separate PR?

@brandyscarney
Copy link
Member

brandyscarney commented Apr 10, 2019

Okay last change sorry. 😂 I updated the icon-only buttons so they have the 50% border-radius and will be rounded on hover. Good to go now!

Screen Shot 2019-04-10 at 5 00 57 PM

@zakton5
Copy link
Contributor Author

zakton5 commented Apr 11, 2019

@brandyscarney Looks great! Nice changes. It'll be nice to finally use these haha

@brandyscarney brandyscarney merged commit 6e1a8f1 into ionic-team:master Apr 11, 2019
@brandyscarney
Copy link
Member

Yay! Merged. 🎉

@simonhaenisch I tried adding us as co-authors but for some reason it didn't work. I actually sent a support message to GitHub about this as I've used the same syntax before without any issues and I'm curious why it didn't work.

@simonhaenisch
Copy link
Contributor

Thanks @brandyscarney, looking forward to try this out soon.

I'm pretty sure you're supposed to have two blank lines before the Co-authored-by comment(s), but I think I also had a case where I did only one and it worked anyway.

@brandyscarney
Copy link
Member

@simonhaenisch Yeah I'm not sure what's going on. The last time I did it was similar and it worked: a7f1f4d

Maybe adding the fixes after broke it? 🤷‍♀️

@zakton5 zakton5 deleted the feat-toast-actions branch April 11, 2019 20:25
@simonhaenisch
Copy link
Contributor

Hm actually I think the issue might be that the "closes ..." comments are below the Co-authored-by comments... might have to be the last line(s) of the message? BTW this is where I read about needing to have two blank lines: https://help.github.com/en/articles/creating-a-commit-with-multiple-authors.

Thanks for trying, however I didn't do all too much here anyway ;)

@brandyscarney
Copy link
Member

Yeah, maybe. I didn't read it that well. 😂 I'm not upset to not be added either but I still want to know what I did wrong. 😭 Maybe they'll message me back, haha we'll see.

@rafalschmidt97
Copy link

rafalschmidt97 commented May 3, 2019

Probably after this PR buttons always use primary colour (with icons and just closeButtonText). I didn't change anything in my project - just updated to the newest version. Should I add some variables into "/** Ionic CSS Variables **/" ?

IMG_2659
IMG_2658

Also please update docs to this pr 🚀

@paulstelzer
Copy link
Contributor

@rafalschmidt97 this should be fixed with PR #18133 I am waiting for that PR merged, too ;)

kiku-jw pushed a commit to kiku-jw/ionic that referenced this pull request May 16, 2019
…am#17147)

Adds a `header` and `buttons` property to toast. This allows for a toast header to be passed and multiple buttons including action buttons and icon only buttons which matches the Material Design spec. Adds hover states to the button to match the spec. Updates usage section to recommend the new way of passing a close button using the buttons array and `cancel` role. If a button is passed using the cancel role default the color to match the spec. Buttons will default to the `end` side but have the option of being placed on the `start` side.

Co-authored-by: Simon Hänisch <simonhaenisch@users.noreply.github.com>
Co-authored-by: Brandy Carney <brandy@ionic.io>

closes ionic-team#16791 closes ionic-team#16237 closes ionic-team#17611
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants