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

Parent activity interface added + missing functions added #195

Merged
merged 11 commits into from Mar 21, 2017
Merged

Parent activity interface added + missing functions added #195

merged 11 commits into from Mar 21, 2017

Conversation

MFlisar
Copy link
Contributor

@MFlisar MFlisar commented Mar 21, 2017

Added a small parent activity callback to forward create view / destroy view events of the slides:

  • id added to slides to distinguish the slides in the callbacks
  • added getter for each important view of the slides so that they can be used in the callbacks

Other small changes:

  • getCurrentSlidePosition added (people may want to scroll to next/previous page or want to handle the back press differently if current position == 0)
  • goToItem manually to scroll to any position

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2017

CLA assistant check
All committers have signed the CLA.

…ntro

# Conflicts:
#	library/src/main/res/layout-land/mi_fragment_simple_slide.xml
#	library/src/main/res/layout/mi_fragment_simple_slide.xml
public Fragment getItem(int position) {
return adapter.getItem(position);
}

@SuppressWarnings("unused")
Copy link
Owner

Choose a reason for hiding this comment

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

See #166 and 8757d41.
It's already implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

@@ -34,7 +35,7 @@
private static final int DEFAULT_PERMISSIONS_REQUEST_CODE = 34; //Random number
private SimpleSlideFragment fragment;


private final int id;
Copy link
Owner

Choose a reason for hiding this comment

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

Normally IDs are saved as long on Android. Not sure if that would make a huge difference, but if it's not too much effort for you, please change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about resource ids as default (mostly, you'll just use the title resource id for this imho), altough using simple ints like 1, 2, 3, ... would be fine as well, I don't want to force anyone to create ids for this... But changing it to long is no problem

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm yes, then just change it to long


import android.view.View;

/**
Copy link
Owner

Choose a reason for hiding this comment

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

Please delete default file template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done as well

}

public LinearLayout getInnerLinearLayout() {
return innerLinearLayout;
Copy link
Owner

Choose a reason for hiding this comment

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

Not really necessary to reveal the internal layout to the public API. This can change soon as I'm adopting ConstraintLayout atm. Just have methods for the TextView's and the ImageView.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need it, I'll add checkboxes and progress bars to this layout and as I saw, this inner layout container is still present in your updated layouts. Really want to remove it?

Copy link
Owner

Choose a reason for hiding this comment

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

If you want to add views and things like that, SimpleSlide is probably not the best way to go. The somplest solution would be a FragmentSlide with your Fragment or layout resource.

Copy link
Owner

Choose a reason for hiding this comment

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

SimpleSlide should keep as simple as possible. Just for basic intros like in the material specs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine. I just think, the SimpleSlide already has all the rotation code in it and can be reused. I can remove the getInnerLinearLayout.

Anyways, following works as well for both layouts (landscape and portrait) currently: LinearLayout ll = (LinearLayout)fragment.getTitleView().getParent();

Copy link
Owner

Choose a reason for hiding this comment

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

A bit hacky though 😄
Basically SimpleSlide is the out of the box solution for say 80% of all intros and when you need forms or inputs or custom layouts, you could either use FragmentSlide with a Fragment or even create a fully customized Slide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. I just don't want to reimplement the fragment for such a simple function. Maybe extending the SimpleSlide to support a CheckBox and ProgressBar via the builder would be a the better alternative

@@ -1275,11 +1275,6 @@ public int getCurrentSlidePosition() {
}

@SuppressWarnings("unused")
public Fragment getItem(int position) {
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't want getItem() to be removed but goToItem() 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my fault, sorry...

Copy link
Owner

Choose a reason for hiding this comment

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

No problem

…de for it

fixed the accidently wrong removed getItem and removed the duplicate goToItem instead
@heinrichreimer
Copy link
Owner

Great!! So everything left is signing the CLA and waiting the Travis build to finish 😃

@MFlisar
Copy link
Contributor Author

MFlisar commented Mar 21, 2017

I already signed it ;-).

@heinrichreimer
Copy link
Owner

Well that's weird.. The bot says it's not signed because it doesn't recognize you as GitHub user...

@MFlisar
Copy link
Contributor Author

MFlisar commented Mar 21, 2017

I see the message here as well, but when clicking details it tells me I have already signed it...

@heinrichreimer
Copy link
Owner

@heinrichreimer
Copy link
Owner

Travis finished successful! 👍

@MFlisar
Copy link
Contributor Author

MFlisar commented Mar 21, 2017

Now I saw it, I somehow have committed with a different email address. Did not know that's possible, my github account does not know it... But found the reason now

@heinrichreimer
Copy link
Owner

@MFlisar you have to link the email address from your commits (mflisar@gmail.com) to your GitHub account. Then the commits should be recognized by GitHub (you can see that by checking if your GitHub avatar appears in the commit view).

@MFlisar
Copy link
Contributor Author

MFlisar commented Mar 21, 2017

I know. I would prefer to change the commit email though. But I'll add it for now.

@heinrichreimer
Copy link
Owner

That would work too

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

3 participants