Topbar v2 by Cool04ek · Pull Request #2413 · wix/react-native-navigation

Cool04ek · 2017-12-26T09:08:39Z

No description provided.

…topbar

guyca · 2017-12-26T09:16:42Z

+        if (options.hidden == False) {
            showTopBar(options.animateHide);
        }
+        if (options.collapse == True) {


This logic isn't a concern of OptionsPresenter. TopBar should have a dedicated class for managing collapse

Sure, I'll move it out as soon it's ready

guyca · 2017-12-26T09:21:09Z

+    @Override
+    public void addView(View child, int index, ViewGroup.LayoutParams params) {
+        super.addView(child, index, params);
+        if (child instanceof ScrollView) {


This means ScrollView must be the root component.

We can look for it recursively, but I'm not sure you need to have collapsing Toolbar if ScrollView is not root

guyca · 2017-12-26T09:26:27Z

+        if (scrollView != null) {
+            if (scrollListener != null) {
+                scrollChangedListener = () -> scrollListener.onScroll(scrollView.getScrollY());
+                scrollView.getViewTreeObserver().addOnScrollChangedListener(scrollChangedListener);


Last time I checked, this method "skips" scroll events. The events reported are kind of batched or posted, meaning the callback isn't invoked immediately after a scroll. This means that if we need to track finger, this method yields subpar results.

From what I've tested, it produces enough events to track finger. I mean we don't need that much precision.

guyca · 2018-01-07T15:27:35Z

-                ViewGroup.LayoutParams layoutParams = container.getLayoutParams();
-				layoutParams.height = ViewGroup.LayoutParams.MATCH_PARENT;
-				container.setLayoutParams(layoutParams);
+				//TODO: needs refactoring


Either refactor now, or remove the comment, open an issue detailing what needs to be refactored and assign your self.

guyca · 2018-01-08T13:49:55Z

+@SuppressWarnings("ResourceType")
+public class NavigationAnimator {
+
+    public interface StackAnimationListener {


Rename this interface as well

guyca · 2018-01-08T13:51:15Z

+            }
+
+            @Override
+            public void onAnimationCancel(Animator animation) {


Switch to adapter and remove unused methods

guyca · 2018-01-08T13:52:25Z

+        AnimatorSet set = new AnimatorSet();
+        set.addListener(new Animator.AnimatorListener() {
+            @Override
+            public void onAnimationStart(Animator animation) {


Use adapter

guyca · 2018-01-08T13:54:20Z

+        set.start();
+    }
+
+    private float getWindowHeight(Context context) {


This method doesn't belong here. It should be pulled into some utils class (ViewUtils?)

guyca · 2018-01-08T14:06:48Z

+        return metrics.heightPixels;
+    }
+
+    public void animateShowTopBar(final TopBar topBar, final View container) {


Seems like NavigationAnimator has two concerns and should be split into two classes.

Animating screens

Animating TopBar

I really dislike the word container... we abused it a lot and would love phasing it out.
Please rename to contentView which is also the naming used in NavigationOptions

guyca · 2018-01-08T14:17:57Z

+            }
+
+            @Override
+            public void onAnimationCancel(Animator animation) {


guyca · 2018-01-08T14:22:35Z

+            }
+
+            @Override
+            public void onAnimationCancel(Animator animation) {


guyca · 2018-01-08T14:27:30Z

+        options.textFontFamily = typefaceManager.getTypeFace(json.optString("textFontFamily", NO_VALUE));
+        options.hidden = NavigationOptions.BooleanOptions.parse(json.optString("hidden"));
+        options.animateHide = NavigationOptions.BooleanOptions.parse(json.optString("animateHide"));
+        options.collapse = NavigationOptions.BooleanOptions.parse(json.optString("hideOnScroll"));


Consider renaming collapse to hideOnScroll? Having the same names across Js and native helps a lot

guyca · 2018-01-08T14:46:21Z

        if (options.hidden == NavigationOptions.BooleanOptions.False) {
            showTopBar(options.animateHide);
        }
+        if (options.drawUnder == True) {


OptionsPresenter is bound to become messy real quick. One way of keeping it clean and readable is to avoid having any kind of logic in it. Ideally, it should act as a delegate and invoke the appropriate style methods of the views.
So contentView won't be a simple view, instead it will be a concrete class or an interface declaring drawBehindTopBar() and drawBelowTopBar(). This change will make the code readable and easier to understand since the user won't need see how the two methods are implemented, to understand what they are doing.
Now, the user needs to read both methods to understand what are these rules that we are adding and removing. It also means that OptionsPresenter makes assumptions on the view and knows it's a direct child of a RelativeLayout.

guyca · 2018-01-08T14:51:28Z

+        }
+    }
+
+    private void showTopBar(NavigationOptions.BooleanOptions animated) {


showTopBar is a detail of TopBar.
OptionsPresenter should only call topBar.showAnimated or topBar.show

guyca · 2018-01-08T14:52:16Z


 @SuppressLint("ViewConstructor")
-public class ContainerLayout extends LinearLayout implements ReactContainer {
+public class ContainerLayout extends RelativeLayout implements ReactContainer {


guyca · 2018-01-08T14:56:06Z

-		this.reactView = reactView;
+    public ContainerLayout(Context context, IReactView reactView, EventDispatcher eventDispatcher) {
+        super(context);
+


What's the purpose of this empty line?

Easier to read. I mean, they belong to different views. And you can distinguish what relates to what

guyca · 2018-01-08T14:56:13Z

+
+        this.topBar = new TopBar(context, this, eventDispatcher);
+        topBar.setId(View.generateViewId());
+


What's the purpose of this empty line?

guyca · 2018-01-08T14:59:50Z

-		addView(reactView.asView());
-	}
+    private void initViews() {
+        LayoutParams layoutParams = new LayoutParams(ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);


import MATCH_PARENT statically

guyca · 2018-01-08T15:33:16Z

        addView(topTabs);
    }
+
+    public void enableCollapse() {


Can we pull this logic into TopBarCollapser, then invoke topBarCollapser.enable()?

guyca · 2018-01-08T15:34:33Z

        super(context);
-        topBar = new TopBar(context, this);
+        this.tabs = tabs;
+        topBar = new TopBar(context, this, null);


Avoid passing null, better to overload and pass null internally.

We should open an issue to implement collapse in topTabs screen

guyca · 2018-01-09T07:13:25Z

+        accelerateInterpolator = new AccelerateInterpolator();
+    }
+
+    public void animateShowTopBar(final TopBar topBar, final View contentView) {


Both TopBar and contentView should be params of TopBarAnimator. This way you're end up with a class which exposes public commands that only accept variables that describe the command, like start translation and duration. This makes the code much easier to understand.

guyca · 2018-01-09T07:17:42Z

+        topbarAnim.start();
+    }
+
+    public void animateHideTopBar(final TopBar topBar, final View contentView) {


Both params should be class members, and the function can be named to hide as we currently only animate the topBar.
topBarAnimator.animateHideTopBar -> topBarAnimator.hide() 👍

guyca · 2018-01-09T07:17:59Z

+        animateHideTopBar(topBar, contentView, 0, accelerateInterpolator, DURATION_TOPBAR);
+    }
+
+    public void animateHideTopBar(final TopBar topBar, final View container, float startTranslation, TimeInterpolator interpolator, int duration) {


Change to private?

guyca · 2018-01-09T07:22:03Z

+import com.reactnativenavigation.utils.UiThread;
+import com.reactnativenavigation.views.TopBar;
+
+public class TopbarCollapsingBehavior {


Change tense to present.
TopBarCollapsingBehavior -> TopBarCollapseBehavior

guyca · 2018-01-09T07:22:23Z

+        this.animator = new TopBarAnimator();
+    }
+
+    public void enableCollapsing() {


enableCollapsing -> enableCollapse

guyca · 2018-01-09T07:58:09Z

            showTopBar(options.animateHide);
        }
+        if (options.drawUnder == True) {
+            reactContainer.drawUnderTopBar();


We need to rename under to behind. Apparently under can mean two things (depending on how you look at things) and behind means exactly what we mean - a view behind a view

guyca · 2018-01-09T07:59:47Z

-		}
-	}
+    private void showTopBar(NavigationOptions.BooleanOptions animated) {
+        topBar.show(animated, contentView);


guyca · 2018-01-09T08:02:52Z

+
+    @Override
+    public void drawUnderTopBar() {
+        //TODO: implement later


Please avoid leaving TODO comments. Open an issue instead 👍

guyca · 2018-01-09T08:03:22Z

+    }
+
+    @Override
+    public void drawUnderTopBar() {


Rename to drawBehindTopBar

guyca · 2018-01-09T08:04:02Z

    private static final String TOP_BAR_FONT_FAMILY = "HelveticaNeue-CondensedBold";
    private static final Typeface TOP_BAR_TYPEFACE = Typeface.create("HelveticaNeue-CondensedBold", Typeface.BOLD);
    private static final NavigationOptions.BooleanOptions TOP_BAR_HIDDEN = True;
+    private static final NavigationOptions.BooleanOptions TOP_BAR_DRAW_UNDER = True;


Rename to behind

Cool04ek and others added 12 commits December 5, 2017 11:52

readme

ad2dce8

merge

929f7d9

doc

47df89b

topbar fix

b55c450

rm doc

2e815f0

Merge branch 'v2' into topbar

dc739e5

Merge branch 'v2' into topbar

51489d0

options presenter fix

ac0e9df

Merge branch 'topbar' of github.com:wix/react-native-navigation into …

e3f3316

…topbar

rm userless stuff

e4f534a

WIP

44c2813

WIP

f3d26b9

Cool04ek added the v2 label Dec 26, 2017

Cool04ek requested a review from guyca December 26, 2017 09:08

guyca reviewed Dec 26, 2017

View reviewed changes

Cool04ek added 14 commits December 26, 2017 16:30

WIP

ccd2170

translation

a2c9101

WIP

3a94214

todo

e53a34e

draw under

918981b

merge

5e3dc19

fix merge

9857169

merge fix

0bd6e12

more merge fix

894c643

improve calc

374272f

animation improvement

e124037

fix tests

7339006

refactor

68e8299

refctor

59193ad

Cool04ek changed the title ~~[WIP] Topbar v2~~ Topbar v2 Jan 5, 2018

Cool04ek added 2 commits January 7, 2018 15:50

fix test

98ef654

anim improve

702d1ec

guyca requested changes Jan 7, 2018

View reviewed changes

rm outdated

ccf545f

Cool04ek mentioned this pull request Jan 8, 2018

ReactRootView height should not be affected when animating TopBar #2378

Closed

guyca requested changes Jan 8, 2018

View reviewed changes

refactoring

0ea0d2b

guyca requested changes Jan 8, 2018

View reviewed changes

Cool04ek added 5 commits January 8, 2018 17:52

more refactoring

b085972

tests

f27bfd6

refactor

44478ca

hotfix

3a377a4

more refactoring

bf7575f

guyca requested changes Jan 9, 2018

View reviewed changes

Cool04ek added 4 commits January 9, 2018 11:34

refactor

726900e

some more refactoring

4002f48

refactor

cf6a0e7

Merge branch 'v2' into topbar

66014ef

guyca previously approved these changes Jan 9, 2018

View reviewed changes

guyca removed the request for review from DanielZlotin January 9, 2018 12:41

Cool04ek dismissed guyca’s stale review via e6ab5de January 9, 2018 13:27

Cool04ek force-pushed the topbar branch from 71006ae to 66014ef Compare January 9, 2018 13:36

Cool04ek added 2 commits January 9, 2018 15:39

Collapse topbar

e379be1

Merge branch 'v2' into topbar

799ad30

guyca approved these changes Jan 9, 2018

View reviewed changes

guyca merged commit a9e2068 into v2 Jan 9, 2018

guyca deleted the topbar branch January 9, 2018 14:23

This was referenced Jul 13, 2021

[Snyk] Upgrade @typescript-eslint/eslint-plugin from 3.3.0 to 3.10.1 Zintexiss/react-native-navigation#3

Closed

[Snyk] Upgrade @typescript-eslint/parser from 3.3.0 to 3.10.1 Zintexiss/react-native-navigation#4

Closed


		this.topBar = new TopBar(context, this, eventDispatcher);
		topBar.setId(View.generateViewId());

Conversation

Cool04ek commented Dec 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers