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

feat(android): support touch feedback for all backgrounds or no background #11795

Closed
wants to merge 2 commits into from

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Jun 24, 2020

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

Summary:

  • The Ti.UI.View properties touchFeedback and touchFeedbackColor used to only show a tap ripple effect when a backgroundColor was applied to the view. This was a documented limitation.
  • Now shows touch feedback ripple to the following as well:
    • backgroundImage
    • backgroundGradient
    • backgroundColor set to TRANSPARENT
    • no background
  • Note that touch feedback is only applied to a view's background.

Test:

  1. Build and run the below on Android 5.0 or higher.
  2. Tap on "Transparent Background" and verify a yellow ripple appears.
  3. Tap on "White Background" and verify a yellow ripple appears.
  4. Tap on "Gradient Background" and verify a yellow ripple appears.
  5. Tap on "Image Background" and verify a yellow ripple appears only on the image.
  6. Tap on "Custom Button" and verify a yellow ripple appears within the blue button.
var window = Ti.UI.createWindow();
var scrollView = Ti.UI.createScrollView({
	layout: "vertical",
	scrollType: "vertical",
	showHorizontalScrollIndicator: false,
	shorVerticalScrollIndicator: true,
	width: Ti.UI.FILL,
	height: Ti.UI.FILL,
});
scrollView.add(Ti.UI.createLabel({
	text: "\nTransparent\nBackground\n",
	textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
	touchFeedback: true,
	touchFeedbackColor: "yellow",
	top: "10dp",
	width: "75%",
}));
scrollView.add(Ti.UI.createLabel({
	text: "\nWhite\nBackground\n",
	textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
	color: "black",
	backgroundColor: "white",
	touchFeedback: true,
	touchFeedbackColor: "yellow",
	top: "10dp",
	width: "75%",
}));
scrollView.add(Ti.UI.createLabel({
	text: "\nGradient\nBackground\n",
	textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
	color: "white",
	backgroundGradient: {
		type: "linear",
		startPoint: { x: "0%", y: "50%" },
		endPoint: { x: "100%", y: "50%" },
		colors: [ { color: "white", offset: 0.0}, { color: "blue", offset: 1.0 } ],
	},
	touchFeedback: true,
	touchFeedbackColor: "yellow",
	top: "10dp",
	width: "75%",
}));
scrollView.add(Ti.UI.createLabel({
	text: "\nImage\nBackground\n",
	textAlign: Ti.UI.TEXT_ALIGNMENT_CENTER,
	color: "green",
	backgroundImage: "/appicon.png",
	touchFeedback: true,
	touchFeedbackColor: "yellow",
	top: "10dp",
	width: "100dp",
	height: "100dp",
}));
scrollView.add(Ti.UI.createButton({
	title: "Custom Button",
	backgroundImage: "android.resource://android/drawable/panel_picture_frame_bg_focus_blue",
	touchFeedback: true,
	touchFeedbackColor: "yellow",
	top: "10dp",
}));
window.add(scrollView);
window.open();

- Used to only be supported when using "backgroundColor" property.
- Now applied to all background types such as:
  * backgroundImage
  * backgroungGradient
  * No background
@build
Copy link
Contributor

build commented Jun 24, 2020

Messages
📖

💾 Here's the generated SDK zipfile.

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

✅ All tests are passing
Nice one! All 6649 tests are passing.
(There are 698 skipped tests not included in that total)

Generated by 🚫 dangerJS against 52795df

@build build requested review from a team June 24, 2020 23:47
@build build added the docs label Jun 24, 2020
*/
private void applyTouchFeedback(@NonNull Integer backgroundColor, @Nullable Integer rippleColor)
private void applyTouchFeedback(Integer rippleColor)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to drop the @Nullable? I kind of like having it be explicit in our APIs so we can use tools to verify checking for null pointers or not...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I question the value of the @Nullable annotation. All reference types are implicitly nullable. The only benefit it provides is more for IDE intellisense purposes since it reveals the annotations and effectively documents the API which we can already do via JavaDoc comments.

The @NonNull annotation is far more valuable since we can use it to trigger compiler/linter errors. So, if it doesn't have the @NonNull annotation, then it's assumed nullable.

Anyways, that's my 2 cents. Unless you can think of something else that might benefit from this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that things are implicitly @Nullable (so long as they're not primitives). I saw it more as a marker of what APIs we've "done" since we've got this large legacy codebase that we didn't use the annotations on. If we don't use @Nullable, it's unclear if we've already reviewed the API and decided they're all implicitly nullable, or if we simply haven't gotten around to adding annotations.

}
});

it.android('touchFeedback', (finish) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the effort in creating a test, but I don't think it's really verifying anything since this is an interactive UI change, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. It doesn't verify anything, but it does exercise the native API to make sure it doesn't crash. So, I guess it's better than nothing? 🤷‍♂️

Ideally, QE should test this by hand via a functional/smoke test since this is a visual feature.

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

The changes LGTM. I don't think the test here would achieve anything, unfortunately so likely this just should be added to QE's UI integration suite if possible.

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed, tested using the test case in description and following the mentioned test steps.

Test Environment

MacOS Big Sur: 11.0 Beta
Xcode: 12.0 Beta 
Java Version: 1.8.0_242
Android NDK: 21.3.6528147
Node.js: 12.18.1
""NPM":"5.0.0","CLI":"8.0.0""
API30 Pixel XL2 emulator

@sgtcoolguy
Copy link
Contributor

Fixed conflict and manually merged via rebase on master.

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