-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
add logic to show introduction paragraph is presented differently in the editing view #4790
base: main
Are you sure you want to change the base?
Conversation
Hi @rajparad! Apologies for the review delay on this one. This all looks good from a code perspective - nice work! I will ask our designers to review further in case they have any additional behavior or copy tweaks before merging. Thanks! |
No worries, Thanks for the reply!! let me know if there is any feedback from designers :) |
Hello @tonisevener, There was a feedback from design to show alert. there are 2 ways. Let me know what is your preference from code perspective. I can update accordingly :). Design feedback is here: https://phabricator.wikimedia.org/T212318 |
Hi @rajparad - apologies for the delay. I think we can get pretty close to option #1. You'll want to put in in the correct places in
The design won't match up exactly, but that is okay. You can trigger within loadContent once all the content is loaded in this method, if no issues have cropped up (i.e. no blocked error, no edit notice is displayed, etc.). If an edit notice modal is automatically presented, you also want to try triggering it after that edit notice modal is dismissed. Usually the featured article of the day has an edit notice, if you want to test that and see how it behaves. Hope this helps. Let us know if you have any other questions, thanks! |
# Conflicts: # Wikipedia/iOS Native Localizations/en.lproj/Localizable.strings
Hello @tonisevener, Thanks for the feedback. I have updated the code to show warning. For now it's only show one time for a user. ![]() Here is the link: https://github.com/wikimedia/wikipedia-ios/pull/4790/files Only thing I am curious is it's now only work with English. I am not how to work with localization in project. Let me know if it's fine or not. |
# Fixed Conflicts: # Wikipedia/iOS Native Localizations/en.lproj/Localizable.strings
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.
@rajparad Thanks! A few more changes needed. Also our project changed a bit with some recent merges (a git submodule was deleted, and the name of PageEditorViewController
changed to EditorViewController
. Hopefully this doesn't cause issues when you pull in the latest from main
.
@@ -248,7 +252,10 @@ final class EditorViewController: UIViewController { | |||
} else { | |||
self.addChildEditor(wikitext: wikitextFetchResponse.wikitext, needsReadOnly: needsReadOnly, onloadSelectRange: wikitextFetchResponse.onloadSelectRange) | |||
} | |||
|
|||
if !UserDefaults.standard.didShowInformationEditingMessage && !UserDefaults.standard.isDifferentErrorBannerShown { |
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 is an additional change to the business requirements, stated here - https://phabricator.wikimedia.org/T212318#9928399.
To accomplish each, what I would do is:
Ensure the notice only shows if someone has chosen to "Edit Introduction" from the first edit button, or "Edit Source" from the overflow menu. Currently I see the notice when I start editing a section, and I understand that the difference in ordering should not be an issue with section edits.
For this you can try adding a simple check against this view controller's sectionID
property. If the sectionID == nil or 0, that should target from the paths in the new requirement. You can add it as a conditional here.
The notice disappeared very quickly and I didn't have time to read all of it - is that our normal length for this type of alerts? or could it be extended?
You can update the WMFAlertManager
's showWarningAlert method to allow passing in a custom duration. Then pass in 5 seconds from here. I will kick off another build and will comment on the task for our further review.
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.
Sure I will take a look, and let you know once I am done with changes :)
There were no major conflicts apart from having conflict in Localizable string. I still have that and when I try to resolve I don't see any changes coming from my local machine or from Above resolve conflict option in github. |
@rajparad Yeah, that could be because git treats the files in |
Phabricator: https://phabricator.wikimedia.org/T212318
Notes
Test Steps
Screenshots/Videos