Skip to content

Fix unnecessary update of ra-input-rich-text on edit - #3099

Merged
djhi merged 7 commits into
marmelab:masterfrom
roychoo:fix-ra-input-rich-text
Apr 19, 2019
Merged

Fix unnecessary update of ra-input-rich-text on edit#3099
djhi merged 7 commits into
marmelab:masterfrom
roychoo:fix-ra-input-rich-text

Conversation

@roychoo

@roychoo roychoo commented Apr 5, 2019 •
edited
Loading

Copy link
Copy Markdown

Hi,

in ra-input-rich-text,

whenever the text changes, below is the sequence of events happening.
Original text <p>test</p>

  1. User change text to <p>test1</p>
  2. onTextChange fired
  3. componentDidUpdate fired
  4. update happens as previous prop value is still <p>test</p>, but unecessary
  5. onTextChange fired again.
  6. stops there as the prevProp input value is the same as current prop input value

2nd issue is that, it seems quilljs is always adding additional <p> after this method

this.quill.setContents(
     this.quill.clipboard.convert(this.props.input.value)
)

ONLY for the text that has special characters, i.e ARTÍCULO 1.- El presente reglamento es de observancia obligatoria after you bold ARTÍCULO 1

fixes #3073

@djhi djhi added this to the 2.8.5 milestone Apr 5, 2019
@djhi

djhi commented Apr 5, 2019

Copy link
Copy Markdown
Contributor

Thanks! It seems the tests are failing though

@roychoo
roychoo force-pushed the fix-ra-input-rich-text branch from ee086ae to b38e3ca Compare April 5, 2019 09:20
@roychoo roychoo mentioned this pull request Apr 5, 2019
Closed
@roychoo
roychoo force-pushed the fix-ra-input-rich-text branch from b38e3ca to 661b8a1 Compare April 11, 2019 02:19
@roychoo

roychoo commented Apr 13, 2019

Copy link
Copy Markdown
Author

Hi just want to check, did anyone take the opportunity to review the code?

djhi
djhi approved these changes Apr 13, 2019
View reviewed changes

@djhi djhi left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! I'll wait for another core team member to review before merging though

fzaninotto
fzaninotto requested changes Apr 16, 2019
View reviewed changes
Comment thread test-setup.js
Comment thread packages/ra-input-rich-text/src/index.spec.js Outdated
Comment thread packages/ra-input-rich-text/src/index.spec.js
Comment thread packages/ra-input-rich-text/src/index.spec.js Outdated
});

it('should call text-change event only once when editing', async () => {
const mockFn = jest.fn();

@fzaninotto fzaninotto Apr 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

call it handleChange

@roychoo roychoo Apr 16, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

@djhi djhi Apr 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you sure ? I still see mockFn

@roychoo roychoo Apr 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh sorry I misunderstood, I thought you meant the test description. Will make the required change in another 3 hours.

@roychoo roychoo Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@djhi done.

Comment thread packages/ra-input-rich-text/src/index.spec.js
Comment thread packages/ra-input-rich-text/src/index.spec.js Outdated
Roy Choo added 4 commits April 16, 2019 16:03
Roy Choo
add comments for clarity
4e81a71 
quill require mock on some DOM apis while in test.
Roy Choo
change description of test
733fa03
Roy Choo
fix test to not use quill internal api
ff4f021
Roy Choo
add more comments for shim and test
622d4a4 
1. add reason for shim
2. add reason for using jest fake timers
@djhi
djhi requested a review from fzaninotto April 19, 2019 12:04
djhi
djhi suggested changes Apr 19, 2019
View reviewed changes
Comment thread packages/ra-input-rich-text/src/index.spec.js Outdated
});

it('should call text-change event only once when editing', async () => {
const mockFn = jest.fn();

@djhi djhi Apr 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you sure ? I still see mockFn

@djhi djhi modified the milestones: 2.8.5, 2.8.6 Apr 19, 2019
@djhi
djhi merged commit 8b2cbc2 into marmelab:master Apr 19, 2019
@djhi

djhi commented Apr 19, 2019

Copy link
Copy Markdown
Contributor

Thanks!

@roychoo

roychoo commented Apr 19, 2019

Copy link
Copy Markdown
Author

Thanks for the review and merged

@fzaninotto fzaninotto mentioned this pull request Apr 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fzaninotto fzaninotto fzaninotto approved these changes
+1 more reviewer
@djhi djhi djhi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.8.6

Development

Successfully merging this pull request may close these issues.

ra-input-rich-text breaks when using special characters

3 participants

@roychoo @djhi @fzaninotto