-
Notifications
You must be signed in to change notification settings - Fork 529
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
Style Change: New styles for the Oppia Deprecation Dialog Design #4997
Style Change: New styles for the Oppia Deprecation Dialog Design #4997
Conversation
Hey @adhiamboperes. I have assigned you this PR for a preliminary review before we get to Ben. Go through it and leave some feedback. |
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.
Hi @kkmurerwa, I have requested some changes.
I suggest looking at the wiki sections for working on the UI and dark mode for more clarity on patterns.
app/src/main/res/drawable/positive_button_rounded_border_solid_color_primary.xml
Outdated
Show resolved
Hide resolved
app/src/main/res/drawable/positive_button_rounded_border_solid_color_primary.xml
Outdated
Show resolved
Hide resolved
Hi @adhiamboperes. I have made the suggested changes. You can take another look at it now. I have updated the PR comment at the top as well to feature the side-by-side comparison of the designs. |
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 have left further comments @kkmurerwa, PTAL.
Also do respond to all current comments so that I can resolve them, thanks!
Unassigning @adhiamboperes since the review is done. |
Hi @kkmurerwa, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
Hi @adhiamboperes. I think I forgot to request another review. I have made all the suggested changes and you can have another pass at this PR. |
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.
Thanks @kkmurerwa! These changes LGTM.
Please respond to all review comments so that I can resolve them before merge.
Unassigning @adhiamboperes since they have already approved the PR. |
Hi @kkmurerwa, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Hi @adhiamboperes. I think I have responded to all conversations. |
I think you're seeing this: #4874 (comment) |
Hi @kkmurerwa, 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. |
Hi @kkmurerwa, 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. |
Hi @adhiamboperes. Should I resolve the conversation or reviews that I inadvertently started while replying to comments or should I just resolve the conversations? |
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.
Here are the review comments.
Explanation
The introduction of the new deprecation dialogs for the App/OS Deprecation feature led to a minor redesign of the dialogs. This includes style changes to the dialogs that will display various deprecation messages. The proposed dialogs can be seen on this Figma link. When merged, this PR will introduce the new theme needed to achieve the desired dialog designs.
The difference between the current and the proposed dialog are highlighted below
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: