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

[New Designs] Update Run button from dashboard to display new flow for selecting desired tests #632

Merged
merged 24 commits into from Dec 14, 2023

Conversation

aanorbel
Copy link
Contributor

@aanorbel aanorbel commented Nov 2, 2023

Fixes ooni/probe#2589

build: https://github.com/ooni/probe-android/suites/18778645278/artifacts/1094292643

Proposed Changes

  • Add RunTestsActivity and related components
. .
Screenshot_20231103_151029 Screenshot_20231103_151057
Screenshot_20231103_151045 Screenshot_20231103_151112

Base automatically changed from chore/gradle-kotlin-upgrade to master November 3, 2023 07:33
@aanorbel aanorbel marked this pull request as ready for review November 3, 2023 14:13
Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me. I had to ask some questions about some language features. Additionally, it seems to me we can remove a bunch of commented out lines and we should add some JavaDoc-like documentation.



private fun updateStatusIndicator() {
//TODO(aanorbel): translate status indicator
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do this in the same PR or in a subsequent one?

engine/build.gradle Show resolved Hide resolved
@aanorbel aanorbel changed the base branch from master to dev/design-update November 24, 2023 14:13
String tn,
@Nullable List<String> urls,
String origin) {
for (AbstractSuite suite : TestAsyncTask.getSuites())
Copy link
Member

Choose a reason for hiding this comment

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

Please, always use { and } even if there is just a single statement afterwards.

for (AbstractSuite suite : TestAsyncTask.getSuites())
for (AbstractTest test : suite.getTestList(app.getPreferenceManager()))
if (test.getName().equals(tn)) {
if (urls != null)
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, { and } 🙏

import org.openobservatory.engine.BaseDescriptor
import org.openobservatory.engine.BaseNettest

class ChildItem(
Copy link
Member

Choose a reason for hiding this comment

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

maybe name this ChildGroupItem?

Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

Once all floating (and minor!) comments have been addresses, we can merge!

🐳

@aanorbel aanorbel merged commit 7b8ec93 into dev/design-update Dec 14, 2023
3 of 5 checks passed
@aanorbel aanorbel deleted the issues/2589 branch December 14, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants