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): hide radio buttons OptionDialog #10645

Merged
merged 10 commits into from Apr 25, 2019
Merged

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Jan 23, 2019

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

Create an OptionDialog without the radio buttons:

var win = Ti.UI.createWindow({
	title: 'Click window to test OptionDialog',
	backgroundColor: 'white'
});

var opts = {
	title: 'Select an option',
	options: ['Option 1', 'Option 2', 'Option 3', 'Option 4'],
	buttonNames: ['Cancel']
}

var dialog;
win.addEventListener('click', function() {
	dialog = Ti.UI.createOptionDialog(opts);

	dialog.addEventListener('click', onSelectDialog);
	dialog.addEventListener('cancel', function(e) {
		alert('Dialog canceled! e.cancel = ' + e.cancel + ', e.index = ' + e.index);
	})

	dialog.show();
});

function onSelectDialog(e) {
	console.log(e);
}

var btn = Ti.UI.createButton({
	title: "toggle selected index",
	bottom: 5
});
btn.addEventListener("click", function() {
	if (opts.selectedIndex == 0) {
		opts.selectedIndex = -1;
	} else {
		opts.selectedIndex = 0;
	}
});
win.add(btn);
win.open();

ti_option

@build
Copy link
Contributor

build commented Jan 23, 2019

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍
📖 ❌ 2 tests have failed There are 2 tests failing and 462 skipped out of 3633 total tests.

Tests:

Classname Name Time Error
ios.Titanium.Media.AudioPlayer .duration 1.003 expected -2147483648 to be within 45250..45500
ios.Titanium.Media.AudioPlayer #start, #stop 2.002 timeout of 2000ms exceeded

Generated by 🚫 dangerJS against 5cee11e

@jquick-axway
Copy link
Contributor

I have different thoughts on how this should be handled. (I've put some thought about this in the past, but haven't done anything about it yet.)

First, I agree that we should not show a list of radio buttons where none of them are checked. It looks ridiculous. By default, it should be a vertical list of buttons. I'm pretty sure that's how most app developers want this to be handled.

However, the OptionDialog does have a "selectedIndex" property which when set will check one of the radio buttons. In this case, using radio buttons are fine. This is also an Android and Windows only property.

So, how about this? Instead of introducing a new "textOnly" property, we could simply check if the "selectedIndex" property is set. If it is, then use radio buttons. If not, then use normal buttons. What do you think?

CC: @hansemannn

@m1ga
Copy link
Contributor Author

m1ga commented Jan 25, 2019

Updated the example and using selectedIndex. If it is -1 (unset) it will display normal text options. Makes sense since a radio button should have at least one selection.
A proper template selection would be nice to add in the future (radio, checkbox, text, images,...) but for now this gives a better parity with iOS in the default case.

@hansemannn
Copy link
Collaborator

Using this in production now, works really great! Who ever reads this, please do not bump it to post 8.1.0 :).

@garymathews garymathews changed the title [AC-6114] Android: Hide radio buttons OptionDialog feat(android): hide radio buttons OptionDialog Mar 1, 2019
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.

CR: PASS

@hansemannn
Copy link
Collaborator

P.S.: Actually just moved to the BottomSheet API since it's even more used in best-practice apps these days.

@m1ga
Copy link
Contributor Author

m1ga commented Mar 1, 2019

yes, looks more like the iOS part. But still it is great to remove the circles when they are not needed for most users 😄

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 Able to see dialog options with and without radio buttons depending on the toggle of the selected index. Tested with the test case mentioned above.

Test Environment

Google pixel xl 7.1.1 sim
APPC CLI: 7.0.10-17
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1

@build
Copy link
Contributor

build commented Apr 22, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍
📖

✅ All tests are passing
Nice one! All 3795 tests are passing.
(There are 466 tests skipped)

Generated by 🚫 dangerJS against 869fb55

@ssjsamir ssjsamir merged commit 324f40e into tidev:master Apr 25, 2019
@m1ga m1ga deleted the optiondialog branch June 20, 2019 12:50
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

8 participants