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

Added DisplayAlert Support #32

Merged
merged 7 commits into from
Nov 18, 2017
Merged

Conversation

SkyeHoefling
Copy link
Contributor

Added basic DisplayAlert Support.

The DisplayAlert API in Xamarin.Forms uses the MessagingCenter to communicate with platform code. This is a simple Subscribe and Unsubscribe including adding and removing the element from the Renderer.

image

Usage

public async void OnButtonPressed(object sender, EventArgs e)
{
    await DisplayAlert("Title", "Some message", "OK");
}

Styles
Following inline with our current UI styles I used Bootstrap styles to build the modal

Current Features

  • Displays modal using boostrap styles
  • Title
  • Message
  • Cancel Button (typically your OK button)
    • This means it currently does not support 2 buttons, right now it is just 1 button
  • X button at the top right corner of the modal
  • Closing the modal by clicking the cancel button or the x button

Still needs implementing
A list of items that need to be implemented still

  • When the API is called with both accept and cancel it needs to return true/false depending on what the user clicks

--
This is where I finished up after playing around with it tonight. I wanted to submit the PR so we can have the code and provide any feedback. I plan to finish up this PR with implementing the return value logic.

@praeclarum
Copy link
Owner

This is great! I was debating whether to use alert() itself, but this looks to be a better approach.

My biggest comment so far has only to do with code formatting. Please make sure your editor obeys the .editorconfig so that all the reformats in Platform.cs go away.

The code looks good now. If you want to keep this PR open to finish that last feature you mentioned, that's fine. Otherwise I can merge and another PR can finish it. Let me know what you prefer.

Try to get a commit in that fixes the formatting if you can. Thanks!

@SkyeHoefling
Copy link
Contributor Author

SkyeHoefling commented Nov 17, 2017

I was thinking the exact same thing of using alert vs bootstrap but since we are using bootstrap all over the place I thought it would be a good place to start. Granted if the bootstrap stylesheet is not referenced this will not work. Perhaps we should add some custom styles to make sure this always works. Unless we are saying Ooui.Forms is going to be dependent on bootstrap.

Code Formatting

  • I have no idea how the code formatting code like this, I'll take care of it and update the PR

Additional Changes

  • Add support for accept/cancel bool return values
  • Clean up modal code and move it into it's own class so it is easier to manage

Build failing

  • Is my current PR okay as far as a build goes?
  • I notice the bitrise build is failing but can't see the output

@praeclarum
Copy link
Owner

praeclarum commented Nov 17, 2017

For now I'm OK to take a dependency on bootstrap. I kept wavering, but I think it's the smart way to go.

In the future, it would be nice to integrate other UI libraries (semantic and material come to mind) - but we'll tackle that when we need it.

I don't really want to get into the business of writing CSS libraries so prefer to leverage existing ones.

You can ignore the build failures for now. I need to work on it and I will fix any breaks this PR may cause (I'm not anticipating any).

@SkyeHoefling
Copy link
Contributor Author

SkyeHoefling commented Nov 17, 2017

@praeclarum regarding the formatting issue: there is an issue with the Platform.cs and it's line endings there is a mismatch of LF and CRLF. It would be best to normalize the line endings, how would you like this done?

image

Since I am using a windows machine in visual studio it automatically is correcting the line endings to CRLF. If this is something we really don't want to fix right now I can try updating it from another machine I have

Andrew Hoefling added 3 commits November 17, 2017 20:28
…in from the Xamarin.Forms Message Center. Implemented a basic modal using the included bootstrap styles
…t with the input of any Xamarin.Forms Page. Added a sample project to test DisplayAlert

(cherry picked from commit dcda89e)
@SkyeHoefling
Copy link
Contributor Author

That was a mild nightmare to fix but I got the line endings all cleaned up in the Platform.cs class. Now I'll work on the rest of my changes

@@ -31,6 +32,19 @@ public class Platform : BindableObject, IPlatform, INavigation, IDisposable
public Platform ()
{
_renderer = new PlatformRenderer (this);

MessagingCenter.Subscribe(this, Page.AlertSignalName, (Page sender, AlertArguments arguments) =>
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We really should put a check into here to return if there is already a DisplayAlert on the screen. I tried adding a simple bool into the platform to handle the state but this ran into a race condition. If you have a second DisplayAlert that displays based on the result of the first the 2nd display alert will not display due to this race condition

@SkyeHoefling
Copy link
Contributor Author

@praeclarum I would say the PR is ready to merge into master. I added a code file comment regarding an issue with multiple DisplayAlert's rendering at the same time and a solution I tried that didn't work.

Let me know if you need me to do more work on this

@praeclarum
Copy link
Owner

Looks great, thanks again for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants