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
219 stars 80 forks source link

7.2.0+: ripple not correctly applied on Android using borderRadius #451

Closed felixkrautschuk closed 1 year ago

felixkrautschuk commented 1 year ago

Make sure to check the demo app(s) for sample usage

Make sure to check the existing issues in this repository

If the demo apps cannot help and there is no issue for your problem, tell us about it

Please, ensure your title is less than 63 characters long and starts with a capital letter.

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.

Describe the steps to reproduce it.

After upgrading plugin version 7.1.4 to 7.2.4, I notice that the ripple is not correctly applied on Android when using a borderRadius. It was working correctly before updating the plugin on Android and it is still working correctly using the latest version on iOS.

That's the layout:

<GridLayout rows="*">
        <mdr:Ripple className="btn" tap="test">
            <Label text="Test"/>
        </mdr:Ripple>
</GridLayout>

and the CSS:

.btn {
    height: 40;
    width: 120;
    ripple-color: rgba(0, 0, 0, 0.2);
    border-radius: 50%;
}

The result: Android left, iOS right https://github.com/nativescript-community/ui-material-components/assets/6443021/616b5125-3488-4b54-9c5e-14c4f1576623

Is there any code involved?

the complete sample:

ns-ripple-issue-android.zip

farfromrefug commented 1 year ago

@felixkrautschuk just released a fix BTW the ripple component is kind of unnecessary in most cases. Follow this then rippleColor can be applied on any view (the label in the example code). It will faster

felixkrautschuk commented 1 year ago

@farfromrefug thanks for the quick fix, now it works as expected.

And I was not aware of that Mixins approach, thank you! However, I notice an issue with that on iOS.

As soon as I call installMixins method in app.ts (without changing anything on the layout or css), the "%" in the border-radius is ignored.

https://github.com/nativescript-community/ui-material-components/assets/6443021/4ab6f1ad-ce99-4fb3-8cc2-e9511e6655b4

On Android, it is still working as expected.

Not such a big deal in my case, as I get the correct result on iOS as well when calculating the half of the height myself and apply the fix number tot the border-radius:

height: 40;
border-radius: 20;

But maybe you could fix this.

Updated sample app: ns-ripple-issue-android.zip

farfromrefug commented 1 year ago

@felixkrautschuk thanks i ll take a look at it

farfromrefug commented 1 year ago

@felixkrautschuk i fixed in the latest version. However there is one bug i cant seem to find a fix for. If you use mixins with non uniform borderRadius + elevation then the elevation is not showing (it is clipped). Tricky to fix

felixkrautschuk commented 1 year ago

@farfromrefug thanks for providing the fix. It is working now on iOS in the sample app. However, when using the latest version (2.5.7) in our real app, the app crashes with the following log:

Cannot read properties of null (reading 'layer') undefined TypeError: Cannot read properties of null (reading 'layer') at GridLayout.updateLayers (file: node_modules/@nativescript-community/ui-material-core/index.ios.js:234:0) at GridLayout._onSizeChanged (file: node_modules/@nativescript-community/ui-material-core/index.ios.js:250:0) at derivedCtor. (file: node_modules/@nativescript-community/ui-material-core/index.common.js:32:0) at GridLayout.updateBackground (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:100:0) at GridLayout.layout (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:91:0) at ViewHelper.layoutChild (file: node_modules/@nativescript/core/ui/core/view/view-helper/view-helper-common.js:96:0) at View.layoutChild (file: node_modules/@nativescript/core/ui/core/view/view-common.js:783:18) at Page.onLayout (file: node_modules/@nativescript/core/ui/page/index.ios.js:471:12) at Page.layout (file: node_modules/@nativescript/core/ui/core/view/index.ios.js:88:0) at ViewHelper.layoutChild (file: node_modules/@nativescript/core/ui/core/view/view-helper/view-helper-common.js:96:0) Fatal JavaScript exception - application has been terminated. NativeScript encountered a fatal error: Uncaught TypeError: Cannot read properties of null (reading 'layer') at updateLayers(file: node_modules/@nativescript-community/ui-material-core/index.ios.js:234:0) at _onSizeChanged(file: node_modules/@nativescript-community/ui-material-core/index.ios.js:250:0) at derivedCtor.(file: node_modules/@nativescript-community/ui-material-core/index.common.js:32:0) at updateBackground(file: node_modules/@nativescript/core/ui/core/view/index.ios.js:100:0) at layout(file: node_modules/@nativescript/core/ui/core/view/index.ios.js:91:0) at layoutChild(file: node_modules/@nativescript/core/ui/core/view/view-helper/view-helper-common.js:96:0) at layoutChild(file: node_modules/@nativescript/core/ui/core/view/view-common.js:783:18) at onLayout(file: node_modules/@nativescript/core/ui/page/index.ios.js:471:12) at layout(file: node_modules/@nativescript/core/ui/core/view/index.ios.js:88:0) at layoutChild(file: node_modules/@nativescript/core/ui/core/view/view-helper/view-helper-common.js:96:0) at layoutView(file: node_modules/@nativescript/core/ui/core/view/view-helper/index.ios.js:209:18) at UIViewControllerImpl.viewDidLayoutSubviews(file: node_modules/@nativescript/core/ui/page/index.ios.js:305:21)

I was not able yet to make it reproducable in the sample app.

And yes, I also noticed the missing elevation on Android. Box shadow seems to be working still so I could use that instead.

farfromrefug commented 1 year ago

Will try to reproduce. And for thé élévation bug I was talking about iOS ;)

farfromrefug commented 1 year ago

@felixkrautschuk should be fixed in latest

felixkrautschuk commented 1 year ago

@farfromrefug now it is working, thank you!