feat: Randomize holder id (#76) #76
Conversation
lib/EditorJs.tsx
Outdated
| @@ -20,6 +20,7 @@ export type Props = Readonly<EditorJS.EditorConfig> & Readonly<EditorJsProps> | |||
|
|
|||
| class EditorJsContainer extends React.PureComponent<Props> { | |||
| instance?: EditorJS | |||
| holderID: string | |||
There was a problem hiding this comment.
| holderID: string | |
| holder: string |
This option must remain same name as the original option. It makes editor-js users predict the available options.
There was a problem hiding this comment.
This looks good. Should I add a commit to change this and all the other references from holderID to holder?
There was a problem hiding this comment.
I've made a new commit that I believe addresses all of the issues you've raised in this review. This is my first time contributing to someone else's GH project and going through this PR/review process, so hopefully I've done so correctly.
lib/EditorJs.tsx
Outdated
| const randomString = () => { | ||
| const min = 1000 * 1000 * 1000 | ||
| const max = 9999 * 1000 * 1000 | ||
| return (Math.round(Math.random() * (max - min * min)) + min).toString(36) | ||
| } |
There was a problem hiding this comment.
| const randomString = () => { | |
| const min = 1000 * 1000 * 1000 | |
| const max = 9999 * 1000 * 1000 | |
| return (Math.round(Math.random() * (max - min * min)) + min).toString(36) | |
| } | |
| this.holderID = `editor-js-${Date.now().toString(36)}` |
If you don't mind, how about generating string based on Date object?
There was a problem hiding this comment.
This makes sense, but in theory, couldn't this lead to a collision if two instances are added at the same exact time? What about adding random jitter of about 1000 to the Date.now() value?
Date.now() + Math.floor(Math.random() * 1000)
|
I'm sorry for the late reply! Thanks for your pull request! |
lib/EditorJs.tsx
Outdated
| constructor(props: EditorJsProps) { | ||
| super(props) | ||
|
|
||
| this.holder = `editor-js-${( |
There was a problem hiding this comment.
| this.holder = `editor-js-${( | |
| class EditorJsContainer extends React.PureComponent<Props> { | |
| holder = `editor-js-${...}`; |
Why don't you try using class field? It's simple to initialize
There was a problem hiding this comment.
I've switched to that approach here: 71a71dc
|
I'm waiting for your contribution. I hope you'll be enthusiastic again. 😢 |
I have become very busy this past week, but don't fret. I do intend to push another commit in the next couple of days. |
Jungwoo-An
changed the title
## [1.7.0](1.6.2...1.7.0) (2020-11-28) ### Features * Randomize holder id ([#76](#76)) [ci skip] ([19b1c64](19b1c64))
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Jungwoo-An
changed the title
This is meant to address #11. As I mentioned in the thread I'm not that familiar with the overall code base, but I figured I'd make a PR in case it works for your project.