-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
Conversation
* added ids to SimpleSlide
removed I prefix from interface
…ng the activity after rotation
…ntro Removed conflicting changes, not necessary anymore anyways
…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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; | ||
|
||
/** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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()
😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my fault, sorry...
There was a problem hiding this comment.
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
Great!! So everything left is signing the CLA and waiting the Travis build to finish 😃 |
I already signed it ;-). |
Well that's weird.. The bot says it's not signed because it doesn't recognize you as GitHub user... |
I see the message here as well, but when clicking details it tells me I have already signed it... |
Have you checked your linked GitHub email: https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user |
Travis finished successful! 👍 |
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 |
@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). |
I know. I would prefer to change the commit email though. But I'll add it for now. |
That would work too |
Added a small parent activity callback to forward create view / destroy view events of the slides:
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