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

Fix change to go back to 'fake' modal #62

Merged
merged 4 commits into from
Apr 19, 2022

Conversation

rayzhou-bit
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #62 (b2c2c56) into main (be12d11) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head b2c2c56 differs from pull request most recent head 70d1515. Consider uploading reports for the commit 70d1515 to get more accurate results

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   86.65%   86.67%   +0.01%     
==========================================
  Files          75       75              
  Lines         667      668       +1     
  Branches       77       78       +1     
==========================================
+ Hits          578      579       +1     
  Misses         85       85              
  Partials        4        4              
Impacted Files Coverage Δ
src/editors/containers/TextEditor/pluginConfig.js 100.00% <ø> (ø)
.../EditorContainer/components/EditorFooter/index.jsx 100.00% <100.00%> (ø)
src/editors/containers/EditorContainer/index.jsx 100.00% <100.00%> (ø)
src/editors/data/redux/app/selectors.js 93.10% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

simpleSelectors.blockValue,
]);
});
it('returns true iff unitUrl, blockValue, and editorInitialized are all truthy', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was intentional. From math iff = "if and only if"

Copy link
Contributor

Choose a reason for hiding this comment

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

Or has this behavior changed?

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 thought this was a typo. Let me change back.

Copy link
Contributor

Choose a reason for hiding this comment

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

more importantly, why has this behavior changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was Ben's change. Without the change, the text editor stays in loading mode (isInitialized remains false). I'll have to look a bit deeper to see what editorInitialized was doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

just confused why editorInitialized is being removed-- I assume its becuase editors are no longer tied to tinymce but confused why this happened in this PR in particular?

Copy link
Contributor

@connorhaugh connorhaugh left a comment

Choose a reason for hiding this comment

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

Wait I get the editorintialized thing-- we need to remove it as there is no longer a callback which triggers it once tinymce loads in, and it was a bug fix in the first place. This better promotes the abstraction layers we want, so as long as it works, I'm in.

@rayzhou-bit rayzhou-bit merged commit 5200897 into main Apr 19, 2022
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.

None yet

3 participants