Closed adhiamboperes closed 2 days ago
Hi @adhiamboperes, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!
@theMr17, @BenHenning, I have responded/Addressed your comments. PTAL.
Unassigning @adhiamboperes since a re-review was requested. @adhiamboperes, please make sure you have addressed all review comments. Thanks!
@theMr17, PTAL.
Unassigning @theMr17 since they have already approved the PR.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Thanks for reviewing @BenHenning. Re - mocks: PTAL at https://www.figma.com/design/PnCtQKDR5VWbIlA3Ui7rTa/Oppia---Onboarding-Android-App?node-id=1446-13996&t=vCOOSRs3SvZiKRqi-4
I have made code changes so that the text is not fully covering the icon, which is closer to the mocks, but I'm not sure if this satisfies the "hard borders" comment.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@BenHenning Thanks for flagging. I took a look at the mocks and it seems that the UI here matches what's provided in the mocks. I'll bring this up with the design team and ask if any changes should be made, but I think it's still worth merging this PR now and doing that as a small fix-up later if we decide to change it.
A quick update -- I did chat with the design team about this. They have a short-term suggestion and a long-term suggestion.
One other concern was i18n -- in German, for example, the text would be "Tippen Sie hier, um ein Bild hinzuzufügen" which is somewhat longer. Dropping the rectangle would probably help with that, too.
Please let me know what you think @adhiamboperes @BenHenning. Thanks!
(Also /cc designers @ashley-ya and @malee04 who were part of this discussion.)
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Thanks much @seanlip, @ashley-ya, and @malee04! I really appreciate the quick turnaround. Just to check, when you say "make the background a bit more transparent" which background is that referring to?
@adhiamboperes do you have any thoughts on the short-term suggestion above? Do you think it's feasible & worth trying to incorporate here, or in a follow-up? FWIW I lean toward trying to do it here if the lift is small.
By "background" I mean the part behind the text, i.e. the white/yellow picture (also, by "transparent" I just mean lighter). This is mostly a thought from me and is optional if the black text on top of the picture has sufficient contrast to be read clearly.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@seanlip, @ashley-ya, and @malee04!, @BenHenning, Here are some attempts to incorporate the feedback. Could you please let me know which direction to lean towards?
Short Text | Long Text | |
---|---|---|
Light Background, regular font weight | ||
Light Background, medium font weight | ||
Yellow Background, regular font |
Thanks @adhiamboperes! Personally, I like the second one best (highest contrast), followed by the third one, followed by the first.
@ashley-ya @malee04 @BenHenning WDYT?
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
@seanlip, @ashley-ya, and @malee04!, @BenHenning, Here are some attempts to incorporate the feedback. Could you please let me know which direction to lean towards? Short Text Long Text Light Background, regular font weight Light Background, medium font weight Yellow Background, regular font
I like the second option as well! the medium font with light background.
Note that this is a summarized snapshot. See the CI artifacts for detailed differences.
Fixed the layouts with "option 2", and updated PR screenshots. There are no other pending comments, so enabling auto-merge.
Explanation
Fix Part of #4938: Add a new Activity and associated Fragments and Presenters to allow a new learner to create a profile. This does not include domain changes.
Placeholder tests have been added to ensure navigation tests are not forgotten. These will fail once navigation has been implemented.
Essential Checklist
For UI-specific PRs only
All Tests Passing on Espresso