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 renderChoices of toolbar #73

Merged
merged 2 commits into from Feb 9, 2016

Conversation

Projects
None yet
5 participants
@zhang-z
Contributor

zhang-z commented Feb 5, 2016

The current renderChoices raises warning:

Warning: Use the defaultValue or value props on instead of setting selected on . It can be seen in Console of "https://zenoamaro.github.io/react-quill/". This commit solved the issue with not very elegant code. If it's OK to change the format of toolbar defaultItems a little bit (add "defaultValue" attribute to indicate selected item, instead of "selected:true"), the code could be simpler.

@zenoamaro

This comment has been minimized.

Show comment
Hide comment
@zenoamaro

zenoamaro Feb 5, 2016

I tink this could be simplified a bit, by returning renderChoiceItem(item) here, and storing the mapped items, so that the following map() could be removed too, saving one loop.

zenoamaro commented on src/toolbar.js in a914f97 Feb 5, 2016

I tink this could be simplified a bit, by returning renderChoiceItem(item) here, and storing the mapped items, so that the following map() could be removed too, saving one loop.

@zenoamaro

This comment has been minimized.

Show comment
Hide comment
@zenoamaro

zenoamaro Feb 5, 2016

Owner

Actually, the best way to do this would ptobably be moving the selected property up to the containing object, just like a normal React select:

{ label:'Font', type:'font', selected:'sans-serif', items: [
    { label:'Sans Serif',  value:'sans-serif' },
    { label:'Serif',       value:'serif' },
    { label:'Monospace',   value:'monospace' }
]},

And maybe, at this point, a name like default could be more appropriate than selected.

Owner

zenoamaro commented Feb 5, 2016

Actually, the best way to do this would ptobably be moving the selected property up to the containing object, just like a normal React select:

{ label:'Font', type:'font', selected:'sans-serif', items: [
    { label:'Sans Serif',  value:'sans-serif' },
    { label:'Serif',       value:'serif' },
    { label:'Monospace',   value:'monospace' }
]},

And maybe, at this point, a name like default could be more appropriate than selected.

@zhang-z

This comment has been minimized.

Show comment
Hide comment
@zhang-z

zhang-z Feb 5, 2016

Contributor

Thanks for the quick reply.

That's what I meant by add "defaultValue" attribute to indicate selected item. But this is not compatible with the just released v0.4.0. Do you think it's worth to change the defaultItems' format? If not, I will modify the code as you mentioned to save one loop

Contributor

zhang-z commented Feb 5, 2016

Thanks for the quick reply.

That's what I meant by add "defaultValue" attribute to indicate selected item. But this is not compatible with the just released v0.4.0. Do you think it's worth to change the defaultItems' format? If not, I will modify the code as you mentioned to save one loop

@zenoamaro

This comment has been minimized.

Show comment
Hide comment
@zenoamaro

zenoamaro Feb 5, 2016

Owner

Apologize for that, you are completely right.

Let's implement the backwards-compatible fix for now, and push the correct format to v0.5.0 along with other breaking changes.

Owner

zenoamaro commented Feb 5, 2016

Apologize for that, you are completely right.

Let's implement the backwards-compatible fix for now, and push the correct format to v0.5.0 along with other breaking changes.

@zhang-z

This comment has been minimized.

Show comment
Hide comment
@zhang-z

zhang-z Feb 9, 2016

Contributor

@zenoamaro , I have improved the implementation as you suggested

Contributor

zhang-z commented Feb 9, 2016

@zenoamaro , I have improved the implementation as you suggested

zenoamaro added a commit that referenced this pull request Feb 9, 2016

Merge pull request #73 from zhang-z/master
Fix selection technique for toolbar `renderChoices`

@zenoamaro zenoamaro merged commit 2eba0e1 into zenoamaro:master Feb 9, 2016

@zenoamaro

This comment has been minimized.

Show comment
Hide comment
@zenoamaro

zenoamaro Feb 9, 2016

Owner

Amazing! Thanks!

Owner

zenoamaro commented Feb 9, 2016

Amazing! Thanks!

@hasanmumin

This comment has been minimized.

Show comment
Hide comment
@hasanmumin

hasanmumin Mar 18, 2016

Hi,I think the warning still ongoing.
Warning: Use the defaultValue or value props on instead of setting selected on .

hasanmumin commented Mar 18, 2016

Hi,I think the warning still ongoing.
Warning: Use the defaultValue or value props on instead of setting selected on .

@martinsiroky

This comment has been minimized.

Show comment
Hide comment
@martinsiroky

martinsiroky Mar 21, 2016

Could you please release version with this fix?

martinsiroky commented Mar 21, 2016

Could you please release version with this fix?

@uxtx

This comment has been minimized.

Show comment
Hide comment
@uxtx

uxtx Apr 5, 2016

Thanks for this fix (and for the work on this great lib)! Has this build been released yet?

uxtx commented Apr 5, 2016

Thanks for this fix (and for the work on this great lib)! Has this build been released yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment