search

nativescript-community / ui-material-components

Monorepo that contains all of the NativeScript Material Design plugins.
https://nativescript-community.github.io/ui-material-components/
Apache License 2.0
215 stars 80 forks source link

Bottom sheet visuals ara bad and crashes on iOS #57

Closed darkyelox closed 4 years ago

darkyelox commented 4 years ago

Runs great on Android but when tested on iOS i have three problems:

Which platform(s) does your issue occur on?

Please, provide the following version numbers that your issue occurs with:



### Please, tell us how to recreate the issue in as much detail as possible. 
Using the same code as the demo-ng folder.
bradmartin commented 4 years ago

Also running into this: https://github.com/Akylas/nativescript-material-components/blob/master/src/bottomsheet/bottomsheet.ios.ts#L227 on iOS with an Angular app.

Still trying to determine best approach to work around.

bradmartin commented 4 years ago

Took me a few to find the old code I had where I worked with viewcontrollers and dismissing things.

So with NS I think the angular service can drop the requirement to pass in the ViewContainerRef in the show bottom sheet options.

The plugin itself can grab the current viewcontroller by using the frame module (even in angular).


import { topmost } from 'tns-core-modules/ui/frame';

hideNativeBottomSheet() {
  const viewController = topmost().currentPage.ios;
const parentController = viewController;

// that's it unless you wanna dump the `parentController var` and instead use the direct `viewController var` to call the `dismissViewController...` method.
}
bradmartin commented 4 years ago

@henrychavez - curious to hear your thoughts since you did a bit of the angular work I believe on this module :)

I have that workaround in my current ng app and it's working fine to always close the bottom sheet without having the error come up.

Be happy to handle the PR if you think this is acceptable and don't have any concerns with dropping the viewContainerRef option.

bradmartin commented 4 years ago

Actually, I'm wrong, there is no need for even doing that. After looking at the bottomsheet.ios file a bit more. The viewcontroller member of the class is already being set when the bottom sheet is shown and that viewcontroller instance should be what is used during dismiss...

I've confirmed this works and it also avoids an issue I introduced with the frame and that was it closed ALL VCs and I'm showing a bottomsheet with a modal already open so this wasn't acceptable.

bradmartin commented 4 years ago

So just using this.viewController to dismiss does indeed work https://github.com/Akylas/nativescript-material-components/blob/master/src/bottomsheet/bottomsheet.ios.ts#L232 and doesn't seem to introduce any side effects within Angular app and I don't see any reason it would cause issues. Should be a simple PR.

bradmartin commented 4 years ago

The first point here about the top/bottom being too large is linked to this line https://github.com/Akylas/nativescript-material-components/blob/master/src/bottomsheet/bottomsheet.ios.ts#L117 where the `position.top is being added to the layout height. Which adds the large empty space on top and bottom of the layout.

Not certain on an appropriate fix yet, working on something and we'll report back.

bradmartin commented 4 years ago

Potential solution: don't use position.top + when creating the CGSizeMake(). I'm running it with a simple layout on iPhone Xr and position.top = 88 which is why you have the large empty space on top and bottom of the provided NS layout into the bottom sheet.

I'm going to use 

position.top / 2 +
        utils_1.layout.toDeviceIndependentPixels(owner.getMeasuredHeight())

for the height of the CGSizeMake which reduces the empty whitespace on top and bottom of the actual layout by half which is more visually appealing.

Screenshots below show result of dividing position.top / 2

iPhone Xr: Simulator Screen Shot - iPhone Xʀ - 2019-09-25 at 13 20 07

iPhone 6: Simulator Screen Shot - iPhone 6 - 2019-09-25 at 13 20 08

Which is still too much empty space in my opinion but it's improved.

bradmartin commented 4 years ago

This screenshot is with the current code only using position.top + running on iPhone Xr:

Simulator Screen Shot - iPhone Xʀ - 2019-09-25 at 13 24 21

farfromrefug commented 4 years ago

@bradmartin will look into that. But as look back at that code. Might it be that we need to use position.bottom and not position.top? I mean it is to get the bottom safe area handle if i remember correctly. Will look at it right away

bradmartin commented 4 years ago

Yeah you might be right. I forget what bottom was returning. I played around with different values and approaches but I never could get the top empty space removed, it looks better on devices without safe area. The iPhone XR in testing always had a good top empty space 😞 that I couldn't figure out how to accommodate for.

farfromrefug commented 4 years ago

@darkyelox found the reason for the crash. It is fixed and i will release it soon. @bradmartin found the reason for the layout issue. The issue is that the ViewController (BottomSheetUILayoutViewController) is applied a safearea as a normal view controller. Thus the difference on different iphones. What we want is to remove the top safearea layout constraint. I know how to fix it. Working on it right now

farfromrefug commented 4 years ago

@bradmartin fixed! Release soon

Screen Shot 2019-09-26 at 13 20 36
bradmartin commented 4 years ago

Nice. Did you see my notes about the crash also? I ran into that quite a bit and it seems that the viewContainerRef might not be entirely necessary to pass. Curious what you found there also. Thanks again for helping with this.

farfromrefug commented 4 years ago

@bradmartin yes i saw but i dont use Angular at all :s I did fix the error with Trying to hide bottom-sheet view but no parent with viewController specified by using ios.getParentWithViewController. That might be a fix for your comment in a sense.

bradmartin commented 4 years ago

Gotcha. I am gonna play around with next release without that option and see if behavior is fine and let you know if we can drop that as part of the options.

farfromrefug commented 4 years ago

@bradmartin @darkyelox the new release is out

bradmartin commented 4 years ago

@farfromrefug can confirm this fixed the issues on iOS 👍

I'm going to open a new issue for a small feature request that I'm encountering with the changes here regarding the layout for top/bottom that was changed

farfromrefug commented 4 years ago

@darkyelox Can i let you close this one ?

darkyelox commented 4 years ago

@farfromrefug yep, with the last version now everything works, tested on Android and iOS.

flodaniel commented 4 years ago

hello,

i am running nativescript 6.4 and the error with the additional whitespace seems to occur again on the iphone 11 pro max..

image

farfromrefug commented 4 years ago

@Firetrip we are going to need more info ... What versions are you using? A sample repo please?

flodaniel commented 4 years ago

Hey @farfromrefug ,

sorry :) I am using version 3.1.6

this are my dependencies: 


"dependencies": {
    "@angular/animations": "~8.2.0",
    "@angular/common": "~8.2.0",
    "@angular/compiler": "~8.2.0",
    "@angular/core": "~8.2.0",
    "@angular/forms": "~8.2.0",
    "@angular/platform-browser": "~8.2.0",
    "@angular/platform-browser-dynamic": "~8.2.0",
    "@angular/router": "~8.2.0",
    "@mapbox/polyline": "^1.1.0",
    "@nstudio/nativescript-checkbox": "^1.0.0",
    "@nstudio/nativescript-loading-indicator": "^1.0.0",
    "@nstudio/nativescript-pulltorefresh": "^1.0.1",
    "@owen-it/nativescript-uuid": "0.0.4",
    "@turf/turf": "^5.1.6",
    "@types/node": "^12.7.11",
    "class-validator": "^0.11.0",
    "libphonenumber-js": "^1.7.27",
    "nativescript-android-utils": "^1.0.2",
    "nativescript-angular": "^8.21.0",
    "nativescript-appavailability": "^1.3.2",
    "nativescript-camera": "^4.5.0",
    "nativescript-cardview": "^3.2.0",
    "nativescript-carousel": "^6.1.1",
    "nativescript-cfalert-dialog": "^1.0.15",
    "nativescript-fancyalert": "^3.0.9",
    "nativescript-feedback": "^1.3.12",
    "nativescript-geolocation": "^5.1.0",
    "nativescript-google-maps-sdk": "^2.8.0",
    "nativescript-imagecropper": "^1.0.7",
    "nativescript-imagepicker": "^7.1.0",
    "nativescript-iqkeyboardmanager": "^1.5.1",
    "nativescript-local-notifications": "^4.0.1",
    "nativescript-localize": "^4.2.0",
    "nativescript-material-bottomsheet": "^3.1.6",
    "nativescript-menu": "^1.1.3",
    "nativescript-ng-ripple": "^2.0.1",
    "nativescript-permissions": "^1.3.8",
    "nativescript-plugin-firebase": "^10.1.1",
    "nativescript-secure-storage": "^2.6.0",
    "nativescript-theme-core": "~1.0.6",
    "nativescript-ui-listview": "^8.0.1",
    "nativescript-windowed-modal": "^6.0.0",
    "pure-uuid": "^1.5.7",
    "reflect-metadata": "~0.1.12",
    "rxjs": "^6.4.0",
    "tns-core-modules": "^6.4.0",
    "zone.js": "^0.9.1"
  },
  "devDependencies": {
    "@angular/cli": "^8.3.21",
    "@angular/compiler-cli": "~8.2.0",
    "@nativescript/schematics": "^0.7.1",
    "@ngtools/webpack": "~8.2.0",
    "codelyzer": "~4.5.0",
    "nativescript-dev-webpack": "^1.5.0",
    "node-sass": "^4.7.1",
    "tns-platform-declarations": "^6.1.1",
    "tslint": "~5.19.0",
    "typescript": "~3.5.3"
  },

And I am opening the bottomsheet like this

import { BottomSheetOptions, BottomSheetService } from 'nativescript-material-bottomsheet/angular';
.
.
.

const options: BottomSheetOptions = {
      viewContainerRef: this.viewContainerRef,
      context: {
            createTripModi: [
                    {
                        id: 1,
                        name: 'one_way'
                    },
                    {
                        id: 2,
                        name: 'two_way'
                    }
                ]
            }
        };
this.bottomSheet.show(CreateTripModusDeciderComponent, options).subscribe((result) => {
            // DO STUFF
});

and this is the template for my component. which i basically copied from the example.

<ListView [items]="createTripModi" (itemTap)="onTap($event)" separatorColor="white" class="list-group">
    <ng-template let-modus="item">
        <Label class="list-group-item" text="{{modus?.name | L}}"></Label>
    </ng-template>
</ListView>
flodaniel commented 4 years ago

Here is also a give from the issue.

bug_bottomsheet

flodaniel commented 4 years ago

I think i found a fix. i did some console logging in your plugin, when the layoutView method of bottomsheet.ios.js runs.

image what you can see, is that the value of safeAreaPositionTop suddenly changes. (the log is from just a single .show call and I am not sure, why there happen multiple layoutView calls.

I found that if i simply substract the safeAreaPositionTop value when calling: view_1.View.layoutChild(null, owner, marginLeft, top, width + marginLeft, height + top); fixes the issue.

I hotfixed it with the following piece (starting at line 102 in bottomsheet.ios.js):

if (!this.ignoreTopSafeArea) {
    top += safeAreaPosition.top;
} else if (this.ignoreTopSafeArea && safeAreaPosition.top !== 0) {
    top -= safeAreaPosition.top;
}

I tested it on an iPhone 11 Pro Max and an iPhone 7 simulator. Maybe it is caused by the BottomNavigation element, that is the basis for my navigation (placed at the bottom of the screen). I call the BottomSheet when one of the TabStripItems of the BottomNavigation is tapped.

farfromrefug commented 4 years ago

@Firetrip have you seen this? Normally everything is possible depending on what you want

flodaniel commented 4 years ago

yeah i have seen it. setting it however has no effect .

flodaniel commented 4 years ago

Also just a minor difference, but I work with an Angular Project, so I import my Options from /angular and not the one you sent. But except the viewContainerReference they are anyway the same :)

JNicolasW commented 4 years ago

If someone still have problem with iOs +X and the bottom sheets are showing big gaps in top or bottom, there is a property in the bottom sheet options call: "ignoreTopSafeArea" and "ignoreBottomSafeArea", these are set to true by default, change to false and the gaps are gone. Affects only iOs