Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(android): fix gradient background support for CardView #11219

Closed
wants to merge 2 commits into from

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Sep 16, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-27258

Description:
Fix support of Gradient background for Ti.UI.Android.CardView. Refactor the assigning of nativeView for TiUICardView in order to avoid duplicated execution of processProperties(). Add some "removed" properties in the docs and remove the color/image states properties. They are currently bugged, but I would not want to expand the scope of this PR.
Note: No unit tests, since this is a bug that is only visual. All the View's events are triggered but it remains invisible.

This PR's test case would be good to be ran again, since there are changes in the code it added:
#10045

Test case:
app.js

var win = Ti.UI.createWindow({layout: 'vertical'}),
	cardView1 = Ti.UI.Android.createCardView({
		width: 200,
		height: 200,
			backgroundGradient: {
			type: 'linear',
			colors: ['red', 'green'],
			startPoint: { x: '50%', y: 0 },
			endPoint: { x: '50%', y: '100%' },
			backFillStart: false
		},
	}),
	cardView2 = Ti.UI.Android.createCardView({
		top: 50,
		width: 200,
		height: 200,
		backgroundColor: 'red'
	});

cardView1.addEventListener('click', function() {
	cardView1.backgroundGradient = {
		type: 'linear',
		colors: ['yellow', 'magenta'],
		startPoint: { x: '50%', y: 0 },
		endPoint: { x: '50%', y: '100%' },
		backFillStart: false
	}
});

cardView2.addEventListener('click', function() {
	cardView2.backgroundColor = 'blue';
});

win.add([cardView1, cardView2]);
win.open();

@ypbnv ypbnv added this to the 8.3.0 milestone Sep 16, 2019
@build build requested review from a team September 16, 2019 14:33
@build
Copy link
Contributor

build commented Sep 16, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4336 tests are passing.
(There are 471 tests skipped)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against de3e711

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to update the statements in:

@Override
public void propertyChanged(String key, Object oldValue, Object newValue, KrollProxy proxy) {
...

To use this.cardView, add PROPERTY_BACKGROUND_GRADIENT handling and remove properties handled by the parent class.

@ypbnv
Copy link
Contributor Author

ypbnv commented Sep 17, 2019

Updated with the changes in propertyChanged(). Also extracted setting the gradient background and extended the test case.

@hansemannn
Copy link
Collaborator

hansemannn commented Sep 17, 2019

@ypbnv Unfortunately, with this patch the borderRadius is not respected anymore. Everything else seems to work fine. Bonus: A ripple effect would be good to work, but thats okay not to and a different scope.

@ypbnv
Copy link
Contributor Author

ypbnv commented Sep 20, 2019

Gradient background does not work together with CardView's border radius. From what I have found this is a problem in native Android as well, unfortunately. Setting any drawable as a background besides the internal RoundRectDrawable will break the corner radius of the native view.

Setting the PR as work in progress because I am still trying to somehow work around this limitation.

As a workaround for now I would suggest creating a "container" view with the desired background gradient and width and height set to Ti.UI.FILL. Putting it as the first child in a CardView and adding any other content to it. Not the best solution performance-wise, but it would allow usage of gradient and border radius.
If your CardView contains only one child, which supports backgroundGradient, then you can take advantage of that and skip the additional view. Set the dimensions of the child to Ti.UI.FILL and set the desired background gradient properties to it as well. After adding it to the card view it will result in the desired look.

Pinging @hansemannn

Both sample codes do not need the changes from this PR.

Sample code with container:

var containerView = Ti.UI.createView({
				width: Ti.UI.FILL,
				height: Ti.UI.FILL,
				backgroundGradient: {
					type: 'linear',
					colors: ['red', 'green'],
					startPoint: { x: '50%', y: 0 },
					endPoint: { x: '50%', y: '100%' },
					backFillStart: false
				}
	}),
	cardView = Ti.UI.Android.createCardView({
				width: 200,
				height: 200,
				borderRadius: 20
});
cardView.add(containerView);

Sample code with a single child, supporting gradient background:

var cardView = Ti.UI.Android.createCardView({
        width: 200,
        height: 60,
        borderRadius: 20,

    }),
    label = Ti.UI.createLabel({
        width: Ti.UI.FILL,
        textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
        height: Ti.UI.FILL,
        text: 'Example',
        color: 'black',
        backgroundGradient: {
            type: 'linear',
            colors: ['cyan', 'magenta'],
            startPoint: { x: 0, y: '50%' },
            endPoint: { x: '100%', y: '50%' },
            backFillStart: false
        },
        font: {
            fontSize: 20,
            fontWeight: 'bold'
        }
    });
cardView.add(label);

@hansemannn
Copy link
Collaborator

It works good! One note: If you also pass a backgroundColor, it will not work.

@hansemannn
Copy link
Collaborator

Unfortunately it broke the backgroundColor usage completely:

var win = Ti.UI.createWindow();

var cardView = Ti.UI.Android.createCardView({
	width: 200,
	height: 200,
	borderRadius: 100,
	backgroundColor: '#333'
});

win.add(cardView);
win.open();

This should render a circle, but it renders a square, since the borderRadius is ignored when combined with a backgroundColor.

@sgtcoolguy sgtcoolguy removed this from the 8.3.0 milestone Dec 10, 2019
@hansemannn
Copy link
Collaborator

Closing for now due to open issues with backwards compatibility and merge conflicts.

@hansemannn hansemannn closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants