AboutActivity - #28
Conversation
…evelop # Conflicts: # app/src/main/AndroidManifest.xml # app/src/main/java/pl/droidevs/books/library/LibraryActivity.java # app/src/main/res/values/strings.xml # app/src/main/res/values/styles.xml
nani92
left a comment
nani92
left a comment
There was a problem hiding this comment.
Great job! Few things to fix. :)
|
|
||
| public void showContributorDetails(ImageView imageView, ConstraintLayout constraintLayout, int index) { | ||
|
|
||
| int FADE_DEFAULT_TIME = 250; |
There was a problem hiding this comment.
Niestety nazwa nie jest zgodna z konwencją 😃
There was a problem hiding this comment.
There was a problem hiding this comment.
Wybacz - mój błąd. Zapomniałem wyciągnąć tą zmienną poza metodę i ustawić jako static final.
|
|
||
| Fragment previousFragment = getSupportFragmentManager().findFragmentById(R.id.fl_about); | ||
| Fragment nextFragment = ContributorFragment.newInstance(); | ||
| FragmentTransaction transaction = getSupportFragmentManager().beginTransaction()/*.setReorderingAllowed(true)*/; |
There was a problem hiding this comment.
w jakim celu tu jest zostawiony zakomentowany kod?
There was a problem hiding this comment.
Bo na początku różnie animacje się zachowywały przy setReorderingAllowed(true). Ale planuję to odkomentować 😉
| import pl.droidevs.books.R; | ||
|
|
||
| /** | ||
| * Created by micha on 09.03.2018. |
There was a problem hiding this comment.
ten komentarz też można by usunąć, jako że nic nie wnosi. Można ustawić w AS żeby w ogóle go nie dodawał
There was a problem hiding this comment.
Wybacz 😔
Zapominam usuwać. A nie przyszło mi w ogóle do głowy aby to wyłączyć.
| public OurTeamRVAdapterViewHolder onCreateViewHolder(ViewGroup parent, int viewType) { | ||
| View view = LayoutInflater.from(parent.getContext()) | ||
| .inflate(R.layout.list_item_contributor, parent, false); | ||
| return new OurTeamRVAdapterViewHolder(view); |
|
|
||
| public void showContributorDetails(ImageView imageView, ConstraintLayout constraintLayout, int index) { | ||
|
|
||
| int FADE_DEFAULT_TIME = 250; |
There was a problem hiding this comment.
Niestety nazwa nie jest zgodna z konwencją 😃
| int FADE_DEFAULT_TIME = 250; | ||
| int MOVE_DEFAULT_TIME = 500; | ||
|
|
||
| Fragment previousFragment = getSupportFragmentManager().findFragmentById(R.id.fl_about); |
There was a problem hiding this comment.
Czy kod odpowiedzialny za animację nie zasługuje na osobną klasę?
| @Override | ||
| public void onBackPressed() { | ||
| ContributorFragment myFragment = (ContributorFragment) getSupportFragmentManager().findFragmentByTag(ContributorFragment.TAG); | ||
| if (myFragment != null && myFragment.isVisible()) { |
There was a problem hiding this comment.
if for if instance of. Kod aż woła o refaktor. Trudno określić co tutaj faktycznie się dzieje.
There was a problem hiding this comment.
Fakt. Poprawiam to właśnie.
| * Created by micha on 13.03.2018. | ||
| */ | ||
|
|
||
| public class AboutVPAdapter extends FragmentPagerAdapter { |
There was a problem hiding this comment.
W takim razie lepszą nazwą wyjdaje się:
AboutViewPagerAdapter
AboutFragmentPagerAdapter
AboutPagerAdapter
| case 1: | ||
| return context.getString(R.string.about_our_team_title); | ||
| } | ||
| return "null"; |
There was a problem hiding this comment.
Co reprezentuje sobą string "null" 😄 ?
There was a problem hiding this comment.
A nie wiem... tak walnąłem w razie jakiegoś wyjątku, żeby nie było wartości null. Ale moje rozumowanie było chyba bez sensu...
There was a problem hiding this comment.
Pusty string byłby wystarczający.
| android:textAlignment="center" | ||
| android:textColor="#000" | ||
| android:textSize="22sp" | ||
| android:textStyle="bold" |
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="16dp" | ||
| android:layout_marginStart="16dp" | ||
| android:background="#fff" |
There was a problem hiding this comment.
Kolory powinny być definiowane w pliku colors.xml. Dobrym rozwiązaniem jest korzystanie ze styli, aby łatwo było w razie potrzebny zmienić cały motyw.
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="16dp" | ||
| android:layout_marginStart="16dp" | ||
| android:layout_marginTop="24dp" |
There was a problem hiding this comment.
Marginesy powinny pochodzić z dimensów.
|
|
||
| </LinearLayout> | ||
|
|
||
| <LinearLayout |
There was a problem hiding this comment.
Nie powinien powstać do tego dedykowany, customy View?
| <string name="see_website">See website</string> | ||
|
|
||
| <!-- Transitions --> | ||
| <string name="iv_avatar_transition_name">iv_avatar</string> |
There was a problem hiding this comment.
Tutaj warto korzystać z atrybutu translatable=false.
3 participants
No description provided.