Skip to content
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 Native Source Editor - Part 1 #4521

Merged
merged 22 commits into from Oct 26, 2023
Merged

Add Native Source Editor - Part 1 #4521

merged 22 commits into from Oct 26, 2023

Conversation

tonisevener
Copy link
Collaborator

@tonisevener tonisevener commented May 5, 2023

Phabricator:
https://phabricator.wikimedia.org/T331936

Notes

This PR adds the native editor feature flag and implements the initial pieces of it within PageEditorViewController. I also added a full source edit button in ArticleViewController, so that we can test performance against a large set of wikitext as we build this. The entry point of this full source edit button is likely to change before release, though.

Note I found a bug in the default automatic theme switching in this branch. Marking as draft while I look into that.

Test Steps

  1. In the Staging scheme, load the article editor, either by tapping the full source "Edit" button at the top of an article or any edit pencil.
  2. Text formatting toolbars and input views should display like our Code Mirror editor, though state logic in the Find and Replace toolbar view will act wrong. I will fix this in a separate find & replace PR later on.

@tonisevener tonisevener requested review from bvibber, a team and staykids and removed request for a team and bvibber May 5, 2023 23:41
@tonisevener tonisevener marked this pull request as draft May 5, 2023 23:42
@tonisevener tonisevener changed the title Add Native Source Editor - Part 1 [HOLD] Add Native Source Editor - Part 1 May 5, 2023
@tonisevener
Copy link
Collaborator Author

tonisevener commented May 6, 2023

Note I found a bug in the default automatic theme switching in this branch. Marking as draft while I look into that.

I can reproduce this bug in main as well, so it's unrelated to these changes. Spun off to bug task T336093.

@tonisevener tonisevener marked this pull request as ready for review May 6, 2023 03:36
@tonisevener tonisevener changed the title [HOLD] Add Native Source Editor - Part 1 Add Native Source Editor - Part 1 May 6, 2023
@staykids staykids self-assigned this May 8, 2023
Copy link
Contributor

@staykids staykids left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soooo cool seeing this come together! I ignored text theming stuff as I assume it's coming in forthcoming PRs. Just a couple of minor notes in PageEditorViewController, but happy to merge as-is.

@@ -1853,6 +1854,7 @@ - (void)updateAppThemeIfNecessary {
if (self.theme != theme) {
[self applyTheme:theme];
[self.settingsViewController loadSections];
[self setWKAppEnvironmentThemeWithTheme: theme];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note for forthcoming PRs that we'd eventually need to also pass along the trait collection changes so the font scaling respects our design intents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - this is updated!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff

func pageEditorDidCancelEditing(_ pageEditor: PageEditorViewController, navigateToURL: URL?)
}

class PageEditorViewController: UIViewController {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mark this as final?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, done!

// MARK: - WKSourceEditorViewControllerDelegate

extension PageEditorViewController: WKSourceEditorViewControllerDelegate {
func sourceEditorViewControllerDidTapFind(sourceEditorViewController: Components.WKSourceEditorViewController) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Components. namespace prefix is unneeded here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@staykids staykids removed their assignment May 8, 2023
Copy link
Collaborator

@mazevedofs mazevedofs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great!

@mazevedofs mazevedofs merged commit 062753c into main Oct 26, 2023
2 checks passed
@mazevedofs mazevedofs deleted the source-editor-1 branch October 26, 2023 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants