-
Notifications
You must be signed in to change notification settings - Fork 301
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: Enhanced layout of "About" Activity #1139
fix: Enhanced layout of "About" Activity #1139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I don't think hosting five different MaterialTextView
s is the way to do it. Please instead take the existing about_text
and turn it into an HTML page. Also, please make sure that the localized variants are also updated. thanks!
Okay, I will get it done. |
Implemented @aaronbrethorst in a new pull request #1148. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I am not a maintainer or stuff.
But I had a look at pr and though maybe my comments could be help.
Nothing too serious, just some thoughts to take with a grain of salt (or a pinch if you're feeling adventurous).
@@ -52,7 +56,14 @@ protected void onCreate(Bundle savedInstanceState) { | |||
|
|||
getSupportActionBar().setDisplayHomeAsUpEnabled(true); | |||
|
|||
TextView tv = (TextView) findViewById(R.id.about_text); | |||
// TextView tv = (TextView) findViewById(R.id.about_text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be better if we could just remove unused code
builder.append("\n\n"); | ||
|
||
tv.setText(builder.toString()); | ||
// builder.append(getString(R.string.about_text)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I personally feel we could just remove this
<string name="translations_sub_heading">Translations:</string> | ||
<string name="contribute_to_our_app_sub_heading">Join the Journey: Contribute to Our App!</string> | ||
<string name="app_description">At OneBusAway, we\'re passionate about creating innovative solutions to simplify your transit experience. Our Android app is a labor of love, made possible by the dedication and talent of a diverse group of individuals and organizations. Here\'s a glimpse into the fantastic contributors who have made OneBusAway what it is today:</string> | ||
<string name="code_contributors_content"><![CDATA[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm not sure, but I I feel its not good to store html in this file, as it would be hard to translate the html anyways
@shankarpriyank I've created a new pull request incorporating the suggested changes made by @aaronbrethorst. |
Ahh sorry, never mind, when I had a look there were no other comments. |
fix #1138
Follow Material UI design pattern.
After:
After.mp4
Please make sure these boxes are checked before submitting your pull request - thanks!
Apply the
AndroidStyle.xml
style template to your code in Android Studio.Run the unit tests with
gradlew connectedObaGoogleDebugAndroidTest
to make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them for the initial submission of the pull request. When addressing comments on a pull request, please push a new commit per comment when possible (reviewers will squash and merge using GitHub merge tool)