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

Prune Editor State Management #9108

Merged
merged 12 commits into from
Feb 4, 2021

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Feb 1, 2021

Related to #9004, OOP refactor of editor.js.

Merge these PRs before this one (in order):

  1. Refactor editor.js with Object-Oriented Principles #9104
  2. Rewrite $E.setState for Single ID Parameter #9107

I think the refactoring is going well enough that we can delete some of the old state management techniques that were very confusing:

  • $D.selected
  • calling $E.refresh in $E.wrap.

I feel confident we can do this because we now have:

  • the newish $E.setState() method, called at appropriate times
  • unique IDs for most comment form elements

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

@noi5e noi5e requested review from a team as code owners February 1, 2021 06:01
@gitpod-io
Copy link

gitpod-io bot commented Feb 1, 2021

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@c134bdd). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9108   +/-   ##
=======================================
  Coverage        ?   82.12%           
=======================================
  Files           ?      100           
  Lines           ?     5931           
  Branches        ?        0           
=======================================
  Hits            ?     4871           
  Misses          ?     1060           
  Partials        ?        0           

@noi5e noi5e added feature explains that the issue is to add a new feature JavaScript outreachy readytomerge labels Feb 1, 2021
$('.rich-text-button').on('click', function(e) {
const { textArea, preview, dSelected } = getEditorParams(e.target);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

completely removed this getEditorParams helper function and everywhere it's called!

e.preventDefault();
if (dSelected) { $D.selected = dSelected; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$D.selected is now gone too!

@noi5e
Copy link
Contributor Author

noi5e commented Feb 1, 2021

This is ready to merge! It's a little difficult to sift through the code review to highlight things, but once the other two PRs are merged, I think it will be easier.

// the legacy editor: /app/views/editor/_editor.html.erb (if it's still in use live?)

const getEditorParams = (targetDiv) => {
const closestCommentFormWrapper = targetDiv.closest('div.comment-form-wrapper'); // this returns null if there is no match
Copy link
Contributor

Choose a reason for hiding this comment

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

This legacy code is not at all good, using ids everywhere is so awesome 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know right!!!

elem.find('.progress-bar-container').eq(0).show();
elem.find('.uploading-text').eq(0).show();
elem.find('.choose-one-prompt-text').eq(0).hide();
console.log($("#image-upload-progress-container-" + $E.commentFormID));
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove console.log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, yeah, I didn't catch this one! I think I remove this in a later PR, but just in case, adding it to my list.

@@ -2,7 +2,7 @@
<%= render :partial => "editor/toolbar", :locals => { :location => :main } %>

<div class="form-group dropzone">
<textarea aria-label="Wiki Text" name="body" tabindex="2" class="form-control" id="text-input" rows="20" cols="60"><% if @node && @node.latest && @node.latest.body %><%= @node.latest.body %><% else %><%= params[:body] %><% end %></textarea>
<textarea aria-label="Wiki Text" name="body" tabindex="2" class="form-control" id="text-input-main" rows="20" cols="60"><% if @node && @node.latest && @node.latest.body %><%= @node.latest.body %><% else %><%= params[:body] %><% end %></textarea>
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 change id in below line to class like you did in above files to keep it consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think it will be okay. This particular instance is in the editor/editor partial, which is the legacy editor that is used in /wiki/new and /wiki/edit... On those pages, there is only one form, so this should be the only id='text-input-main' that's on that page.

On pages with multiple forms for each comment (questions, wikis, & research notes), there should still only be one id='text-input-main' for the comment form at the bottom of the page.

@codeclimate
Copy link

codeclimate bot commented Feb 4, 2021

Code Climate has analyzed commit 919208e and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren jywarren merged commit 2b8db65 into publiclab:main Feb 4, 2021
@noi5e noi5e deleted the prune-editor-state-management branch February 4, 2021 00:40
jywarren pushed a commit that referenced this pull request Feb 4, 2021
* merge branches #9104 #9107 #9108 #9110 #9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test
jywarren pushed a commit that referenced this pull request Feb 6, 2021
* merge branches #9104 #9107 #9108 #9110 #9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test

* add text-input class, data-form-id attribute to textarea #9004 #9131

* add data-form-id to save & recover buttons #9004 #9131

* E.setState when clicking save, recover, & .text-input #9004 #9131

* refactor save & recover #9004 #9131

* add icon to recover button #9004 #9131

* restore edit button tooltip #9004 #9131

* save with window.location.pathname, not href #9004 #9131

* new system test for save & recover #9004 #9131

* update test selector #9004 #9131
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* merge editor refactor branches publiclab#9004

* delete refresh & isSingleFormPage methods publiclab#9004

* delete some D.selected assignments publiclab#9004

* add dropzone class to choose one input elements

* E.setState on image select upload publiclab#9004

* remove D.selected from progress bar handlers publiclab#9004

* add unique IDs to image upload progress bars publiclab#9004

* updated image upload progress bar selectors publiclab#9004

* update progress bar selector publiclab#9004

* update selectors for image upload text publiclab#9004
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test

* add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131

* add data-form-id to save & recover buttons publiclab#9004 publiclab#9131

* E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131

* refactor save & recover publiclab#9004 publiclab#9131

* add icon to recover button publiclab#9004 publiclab#9131

* restore edit button tooltip publiclab#9004 publiclab#9131

* save with window.location.pathname, not href publiclab#9004 publiclab#9131

* new system test for save & recover publiclab#9004 publiclab#9131

* update test selector publiclab#9004 publiclab#9131
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* merge editor refactor branches publiclab#9004

* delete refresh & isSingleFormPage methods publiclab#9004

* delete some D.selected assignments publiclab#9004

* add dropzone class to choose one input elements

* E.setState on image select upload publiclab#9004

* remove D.selected from progress bar handlers publiclab#9004

* add unique IDs to image upload progress bars publiclab#9004

* updated image upload progress bar selectors publiclab#9004

* update progress bar selector publiclab#9004

* update selectors for image upload text publiclab#9004
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test

* add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131

* add data-form-id to save & recover buttons publiclab#9004 publiclab#9131

* E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131

* refactor save & recover publiclab#9004 publiclab#9131

* add icon to recover button publiclab#9004 publiclab#9131

* restore edit button tooltip publiclab#9004 publiclab#9131

* save with window.location.pathname, not href publiclab#9004 publiclab#9131

* new system test for save & recover publiclab#9004 publiclab#9131

* update test selector publiclab#9004 publiclab#9131
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* merge editor refactor branches publiclab#9004

* delete refresh & isSingleFormPage methods publiclab#9004

* delete some D.selected assignments publiclab#9004

* add dropzone class to choose one input elements

* E.setState on image select upload publiclab#9004

* remove D.selected from progress bar handlers publiclab#9004

* add unique IDs to image upload progress bars publiclab#9004

* updated image upload progress bar selectors publiclab#9004

* update progress bar selector publiclab#9004

* update selectors for image upload text publiclab#9004
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test

* add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131

* add data-form-id to save & recover buttons publiclab#9004 publiclab#9131

* E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131

* refactor save & recover publiclab#9004 publiclab#9131

* add icon to recover button publiclab#9004 publiclab#9131

* restore edit button tooltip publiclab#9004 publiclab#9131

* save with window.location.pathname, not href publiclab#9004 publiclab#9131

* new system test for save & recover publiclab#9004 publiclab#9131

* update test selector publiclab#9004 publiclab#9131
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* merge editor refactor branches publiclab#9004

* delete refresh & isSingleFormPage methods publiclab#9004

* delete some D.selected assignments publiclab#9004

* add dropzone class to choose one input elements

* E.setState on image select upload publiclab#9004

* remove D.selected from progress bar handlers publiclab#9004

* add unique IDs to image upload progress bars publiclab#9004

* updated image upload progress bar selectors publiclab#9004

* update progress bar selector publiclab#9004

* update selectors for image upload text publiclab#9004
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
* merge branches publiclab#9104 publiclab#9107 publiclab#9108 publiclab#9110 publiclab#9118

* change #dropzone-large ID to #comment-form-body

* add comprehensive preview button test

* add text-input class, data-form-id attribute to textarea publiclab#9004 publiclab#9131

* add data-form-id to save & recover buttons publiclab#9004 publiclab#9131

* E.setState when clicking save, recover, & .text-input publiclab#9004 publiclab#9131

* refactor save & recover publiclab#9004 publiclab#9131

* add icon to recover button publiclab#9004 publiclab#9131

* restore edit button tooltip publiclab#9004 publiclab#9131

* save with window.location.pathname, not href publiclab#9004 publiclab#9131

* new system test for save & recover publiclab#9004 publiclab#9131

* update test selector publiclab#9004 publiclab#9131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature explains that the issue is to add a new feature JavaScript outreachy readytomerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants