Allow toggling of visible scrollbar indicators#4523
Allow toggling of visible scrollbar indicators#4523vakrilov merged 8 commits intoNativeScript:masterfrom sitefinitysteve:ScrollViewUpdates
Conversation
|
Hey @sitefinitysteve - thanks for the effort. |
|
I know about the new guides, saw your tweet and followed the instructions :)
But running the app and tests all just fail... Not like the tests fail, but
fail to run.
Maybe it's that i need 3.1.1 loaded and I'm only on 3.0 (can't risk an
update breaking things, important app about to ship).
What if i write the test as i assume it would pass... Would the ci here
pick it up and run it?
…On Jul 7, 2017 8:08 AM, "Alexander Vakrilov" ***@***.***> wrote:
Hey @sitefinitysteve <https://github.com/sitefinitysteve> - thanks for
the effort.
We have recently published a contributing guideline
<https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md>.
You can give it a try - it explains how to run and write unit tests.
That said - unit tests are really essential for maintaining the quality of
the project, so we would appreciate if you can write test to test this
feature. There are bunch of other scroll-view related tests here
<https://github.com/NativeScript/NativeScript/blob/master/tests/app/ui/scroll-view/scroll-view-tests.ts>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4523 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABeI6EvTpIBykY6RfCs21AlI4Xs8jZubks5sLh_BgaJpZM4OQIHw>
.
|
|
run ci |
|
Okay I added a test, I can't get it to run on iOS (per the docs), but seems to pass on Android (I mean fails on someones grid test though... |
|
run ci |
|
I can't see the unit test build failed results... internal server or has to be published back out? |
|
Hi, Thanks for the unit tests.
|
|
Sure, one step further, scrollBarIndicatorVisible? |
|
That's even better. |
|
run ci |
|
|
||
| public _onOrientationChanged() { | ||
| // NOOP | ||
| this.updateScrollBarVisibility(this.scrollBar); |
There was a problem hiding this comment.
Getting TypeScript transpile error here:
error TS2339: Property 'scrollBar' does not exist on type 'ScrollView'.
Should be this.scrollBarIndicatorVisible
| return 0; | ||
| } | ||
|
|
||
| get scrollBarIndicatorVisible(): boolean { |
There was a problem hiding this comment.
Can you make this just:
public scrollBarIndicatorVisible: boolean;
This property will be re-defined when registering the property, so returning a value can confuse someone that this is the actual implementation.
There was a problem hiding this comment.
It is similar to the orientation property in the beginning of the class - you can also move it there
| orientationProperty.register(ScrollViewBase); | ||
|
|
||
| export const scrollBarIndicatorVisibleProperty = new Property<ScrollViewBase, boolean>({ | ||
| name: "scrollBar", defaultValue: true, |
There was a problem hiding this comment.
Change the name -> "scrollBarIndicatorVisible"
| this.waitUntilTestElementLayoutIsValid(); | ||
|
|
||
| if (app.ios) { | ||
| TKUnit.assertEqual(this.testView.ios.showsHorizontalScrollIndicator, true); |
There was a problem hiding this comment.
Use nativeView instead of ios/android
|
Hi @sitefinitysteve If you are still having problems running the tests on both platforms - just ping me on slack to get this sorted. Being able to write and run the unit test is an essential prerequisite to a successful PR. |
|
Sure, i swear to god on Android all the tests passed, i did run it. :/
…On Jul 11, 2017 8:55 AM, "Alexander Vakrilov" ***@***.***> wrote:
Hi @sitefinitysteve <https://github.com/sitefinitysteve>
I've requested some changes. After the rename the project isn't building.
It is a good idea to build the project and make sure all tests are green
when pushing in the PR branch.
If you are still having problems running the tests on both platforms -
just ping me on slack to get this sorted. Being able to write and run the
unit test is an essential prerequisite to a successful PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4523 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABeI6Jx76xUP2yz-uVD6JoIVXIUkJQfjks5sM3DYgaJpZM4OQIHw>
.
|
|
run ci |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
Implements #4522
Okay, so I wasn't able to get the tests running here locally... kept crashing. I took the source and put it in a hello world to test with.