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

PreferredContentSize #1937

Merged
merged 4 commits into from
Jun 11, 2017
Merged

PreferredContentSize #1937

merged 4 commits into from
Jun 11, 2017

Conversation

g0rdan
Copy link
Contributor

@g0rdan g0rdan commented Jun 10, 2017

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Feature

⤵️ What is the current behavior?

Now you can use 4 parameters in MvxModalPresentationAttribute

🆕 What is the new behavior (if this is a feature change)?

PR will add new, fifth paramater - PreferredContentSize. It can set content size of your modal view.

💥 Does this PR introduce a breaking change?

Nope

🐛 Recommendations for testing

I'm not sure if it works on iPhone because minimum width of content size should be 320. It definitely works on iPad.

📝 Links to relevant issues/docs

Official doc

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated (docs style guide)
  • Nuspec files were updated (when applicable)
  • Rebased onto current develop

@g0rdan g0rdan changed the title Develop PreferredContentSize Jun 10, 2017
@Cheesebaron
Copy link
Member

Thank you for your Pull Request. Please use the Pull Request template, when creating a Pull Request. You can find it here: https://github.com/MvvmCross/MvvmCross/blob/develop/.github/PULL_REQUEST_TEMPLATE.md

@Cheesebaron Cheesebaron added the p/ios iOS platform label Jun 10, 2017
@g0rdan
Copy link
Contributor Author

g0rdan commented Jun 11, 2017

@Cheesebaron Sure, done.

@Cheesebaron
Copy link
Member

Looks good to me. Can someone else also review this please?

@nmilcoff
Copy link
Contributor

@Cheesebaron 👍

@mvanbeusekom
Copy link
Contributor

I like the feature and it has an obvious use case. Just to note that we also solved similar options through adding an interface. This might be a nice alternative since this option is only applicable for iPad and adding it to the attribute might confuse developers in case of iPhone solutions

@Cheesebaron Cheesebaron merged commit fdb7acf into MvvmCross:develop Jun 11, 2017
@Cheesebaron
Copy link
Member

@mvanbeusekom it is not obvious how to do exactly that reading the documentation.

@martijn00 martijn00 added this to the 5.0.3 milestone Jun 11, 2017
@mvanbeusekom
Copy link
Contributor

@Cheesebaron can you elaborate which part is not obvious, maybe we can improve the documentation?

@Cheesebaron
Copy link
Member

Reading the docs about overriding the presentation attributes at runtime: https://www.mvvmcross.com/documentation/platform/ios-view-presenter?scroll=720#override-a-presentation-attribute-at-runtime It only allows you to pass along presentation attributes. So passing along your own presentation attributes here wont work either. Until, you actually override the presenter too, which then responds to your custom attributes.

It is a lot more work, compared to just have out of the box support for a PreferredContentSize property, which might only work on iPad.

The docs may be fine for this and readers smart enough would know how to combine both overriding the presenter and adding their own attributes. However, I would love to see a more in depth example, which is not just using the default attributes.

@mvanbeusekom
Copy link
Contributor

mvanbeusekom commented Jun 12, 2017

I see what you mean, unfortunately what I mean is not documented (so this will be a nice task). For example to pas additional settings to the Xamarin Sidebar implementation you can implement the IMvxSidebarMenu interface. This will allow you to add additional behaviour to the MvxSidebarPresentationAttribute. In this case these are settings like:

  • AnimateMenu
  • DisablePanGesture
  • DarkOverlayAlpha
  • HasDarkOverlay
  • HasShadowing
  • MenuButtonImage
  • etc...

These are all similar to the settings added by this pull-request, hence my suggestion to keep things in sync.

@Cheesebaron
Copy link
Member

Alright. We can undo this one. We just need to make it more clear in the docs how to do these things.

@mvanbeusekom
Copy link
Contributor

mvanbeusekom commented Jun 12, 2017

I'll start writing some additional documentation. I needed I can also convert this PR into an interface, but maybe @g0rdan prefers to do it himself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p/ios iOS platform
Development

Successfully merging this pull request may close these issues.

5 participants