-
Notifications
You must be signed in to change notification settings - Fork 16
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: Text and icons adjustment project #364
feat: Text and icons adjustment project #364
Conversation
Thanks for the pull request, @IvanStepanok! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
c63386d
to
c605f7e
Compare
@IvanStepanok, we need to check that we are using the text "Log out" and not "Logout" on the popup. |
@IvanStepanok correct text |
de503fd
to
a71c8a4
Compare
a71c8a4
to
a4da518
Compare
if viewModel.courseState() != .enrollOpen { | ||
OfflineSnackBarView(connectivity: viewModel.connectivity, | ||
reloadAction: { | ||
await viewModel.getCourseDetail(courseID: courseID, withProgress: false) | ||
}) | ||
} |
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.
Question: Shouldn't the OfflineSnackBarView
only appear if there's no internet connection? The condition seems to be checking something different.
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.
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.
Light and dark icons are added for most of icons. How about adding just one icon and then apply the color through code. This will help the open source community to configure the app according to their theme requirements.
- In figma, there isn't any cross button.
- Cross button is missing as per Figma file
- The icon is not properly visible in light mode.
Core/Core/View/Base/AlertView.swift
Outdated
@@ -268,19 +268,19 @@ public struct AlertView: View { | |||
}, label: { | |||
ZStack { | |||
Text(CoreLocalization.Alert.logout) | |||
.foregroundColor(Theme.Colors.primaryButtonTextColor) | |||
.foregroundColor(.black) |
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.
Using a hardcoded color will limit the theming capability for the open source community. Please define a color in the theme and use that, so anyone can configure it as per their theming requirements.
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.
-
This is a great idea, let's go ahead with it but not as part of the current task.
-
Regarding the cross button, I've remembered it being part of the project from the start, and I'm not sure it should be removed, as it aligns well with the concept of user-friendly UX/UI. Perhaps it should be included in the design?
-
The icon matches the design, but you're right, I would fix that in the design.
-
I added a new color, alertText
Thanks, Saeed!
Core/Core/View/Base/AlertView.swift
Outdated
.font(Theme.Fonts.labelLarge) | ||
.frame(maxWidth: .infinity) | ||
.padding(.horizontal, 16) | ||
Image(systemName: "rectangle.portrait.and.arrow.right") | ||
.foregroundColor(Theme.Colors.white) | ||
.foregroundColor(.black) |
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 is the case here.
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.
🫡
"COURSEWARE.BACK_TO_OUTLINE" = "Back to outline"; | ||
"COURSEWARE.SECTION" = "Section “"; | ||
"COURSEWARE.IS_FINISHED" = "“ is finished."; | ||
"COURSEWARE.SECTION" = "You've completed “"; |
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.
How about defining SECTION
in such a way that it can take the section name as input? In this way, IS_FINISHED
won't be needed. Assigning .
to IS_FINISHED
is looking good.
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.
🫡
.padding(.bottom, 20) | ||
Spacer() | ||
}.padding(.leading, 10) | ||
|
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.
Extra new line, it can be removed.
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.
🫡
}) | ||
|
||
if viewModel.isShowProgress { | ||
ProgressBar(size: 40, lineWidth: 8) |
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.
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.
Fixed, thanks!
Thanks guys🙌 |
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.
LGTM 👍
@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Figma design changes: https://www.figma.com/file/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?type=design&node-id=9980-2&mode=design&t=ThvK5qvjJxh91mw2-0
Changed icon
Empty state’s title has been removed
Changed subtitle
Internet warning text has been changed
Changed subtitle. “Handouts” changed to “More”
Changed title and icon
Changed title and subtitle
Changed subtitle
The title of the message has been removed.
Changed icon, title, and subtitle