-
-
Notifications
You must be signed in to change notification settings - Fork 255
Refactoring TileEditor to function component #1984
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
base: master
Are you sure you want to change the base?
Refactoring TileEditor to function component #1984
Conversation
|
Hello @tomivm and @RodriSanchez1, can they review this migration? @zkarmi continued with my proposal and finished it. |
| TileEditor.propTypes = { | ||
| intl: PropTypes.object.isRequired, | ||
| open: PropTypes.bool, | ||
| onClose: PropTypes.func.isRequired, | ||
| editingTiles: PropTypes.array, | ||
| onEditSubmit: PropTypes.func.isRequired, | ||
| onAddSubmit: PropTypes.func.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized the block comments for these PropTypes are missing, and that TileEditor.propTypes.intl needs to be changed back from PropTypes.object.isRequired to intlShape.isRequired. I'll revert that with the next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to make this changes pleaseee!
|
Hi @RodriSanchez1 @tomivm , I wanted to put this back on your radar, as I haven't heard anything in several weeks. |
|
Hi @zkarmi ! Sorry for the delay 😅 . Thanks for your amazing work! Answering your questions: Also we have two Board searchers: We should merge both selector and have only one board searcher. Not sure how to deal with this problem. I need to think a bit and talk with the team. We should attact this problem in a next PR |
|
@RodriSanchez1 Thanks for the feedback!
|
|
Awseome! Yes you are right, it doesn't make sense. We can make it |
|
Hi @RodriSanchez1 |
Testing SummaryTested the TileEditor refactor and all functionality is working as expected: Tested Scenarios
Results
The refactor from class component to function component seems to be working fine and no problems detected with the new changes. |


This PR resolves #1964, migrating the TileEditor component from class component to function component.
Thank you @gonojuarez for your work starting this off and explaining some of the behaviors to me.
With this commit I have some questions for whoever can answer, mostly around if we want to refactor some inefficiencies here or if they should wait for another PR:
https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R232-R237
getDefaultColor()can be shorter, andhandleTypeChange()can callgetDefaultColor()to make it shorter too. Should we refactor this?.TileEditor__loadBoard_sectiondiv, there is a check forlinkedBoard === NONE_VALUE. However, the only timelinkedBoardwould beNONE_VALUE(which is the string'none') is ifsetLinkedBoardState()was called and a matching board could not be found. In all other cases wherelinkedBoardis nullified, it is set to an empty string (''). Is this intentional?