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

[proofing] search+replace function #470

Closed
wants to merge 14 commits into from

Conversation

kvchitrapu
Copy link
Collaborator

@kvchitrapu kvchitrapu commented Mar 1, 2023

Feature

Search and replace a string works. May need stylesheet changes to pretty it up. Completes #455.

Tests

Screenshot 2023-03-16 at 8 28 50 PM

Screenshot 2023-03-16 at 8 29 37 PM
Screenshot 2023-03-16 at 8 30 06 PM

@akprasad
Copy link
Contributor

akprasad commented Mar 1, 2023

Thanks for this. Here's the flow I have in mind:

  1. Default page -- user sees a search/replace box, fills it in, and submits.
  2. Preview page -- user sees all of the changes that will be made in the given text and is given an option to confirm
  3. Confirmation page -- when user confirms, we create a new revision for each affected page. On the frontend, we return to (1) and show a message like "Made X replacements across Y pages."

IIUC this PR completes the implementation of (2). Do you feel comfortable trying (3) as well?

@kvchitrapu
Copy link
Collaborator Author

kvchitrapu commented Mar 1, 2023

Thanks for this. Here's the flow I have in mind:

  1. Default page -- user sees a search/replace box, fills it in, and submits.
  2. Preview page -- user sees all of the changes that will be made in the given text and is given an option to confirm
  3. Confirmation page -- when user confirms, we create a new revision for each affected page. On the frontend, we return to (1) and show a message like "Made X replacements across Y pages."

IIUC this PR completes the implementation of (2). Do you feel comfortable trying (3) as well?

You're right. I completed #2.

From our previous exchange on this issue, I understood that the user manually cherrypicks the changes and updates the edit page.

What you listed as #3 is key to a better proofer experience. I'll give it a shot.

@akprasad
Copy link
Contributor

akprasad commented Mar 4, 2023

Thanks -- please feel free to give #3 a shot as part of this PR.

@kvchitrapu
Copy link
Collaborator Author

Thanks -- please feel free to give #3 a shot as part of this PR.

I made the required changes. Now testing and debugging them. I am hoping to send a PR update this weekend.

@kvchitrapu
Copy link
Collaborator Author

Thanks -- please feel free to give #3 a shot as part of this PR.

I made the required changes. Now testing and debugging them. I am hoping to send a PR update this weekend.

@akprasad, I found a code bug on Saturday but haven't had a chance to work on it yet. I'll resolve the issue as soon as possible.

@kvchitrapu
Copy link
Collaborator Author

kvchitrapu commented Mar 17, 2023

@akprasad , I have the end-to-end flow working. Please review. Unittest coverage is not at 80%. I'll check tomorrow.

Potential Improvements
While the overall functionality works, the code quality could be better. I am not familiar with JavaScript and UI code.

  1. UI, colors, checkboxes, and wording can be improved.
  2. The "Check-all/Uncheck-all" boxes need refinement, particularly in JavaScript.
  3. The way changes are carried back and forth between HTML code and server code in form fields can be improved, as there is no pagination available at this time.
  4. There may be better ways to name the form fields for match{{page-slug}}-{{line-number}}-replace to mark each selected phrase for replacement. I chose the simplest implementation.
  5. No translations have been done.

@kvchitrapu kvchitrapu closed this Mar 18, 2023
akprasad pushed a commit that referenced this pull request Mar 24, 2023
Search and replace a string works. May need stylesheet changes to pretty
it up. Completes #455.

Closed #407. Created a new clean branch and as the base branch for #470
pulled an existing commit.

## Tests

See screenshots on PR #476.
@kvchitrapu kvchitrapu deleted the feature-search-replace branch April 2, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants