Refactoring TileEditor to function component
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:
- There is duplicate code in at least 3 places to reset 6 state variables. Can we extract this to a single function in this PR? https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R232-R237
setActiveStep(0);
setSelectedBackgroundColor('');
setTile(defaultTile);
setImageUploadedData([]);
setIsEditImageBtnActive(false);
setLinkedBoard('');
getDefaultColor()can be shorter, andhandleTypeChange()can callgetDefaultColor()to make it shorter too. Should we refactor this?- In the
.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?
Hello @tomivm and @RodriSanchez1, can they review this migration? @zkarmi continued with my proposal and finished it.
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:
1- Yes, lets extract that piece of code to avoid unnecesary repetitions.
2- Yes, refactor it please. It can be cleaner for sure!
3- Iam not have a response now. I was investigating it, but is a bit complex. The application have a mix between the linkedBoard and loadBoard. The linkedBoard suppose to be a boolean and the loadBoard is a string that contains the id of the related board.
Also we have two Board searchers:
The first selector shows the user board filtered.
The second one was the latest that we added. It works only for the browser app (not cordova). It shows all the boards that an user has without any filtering. We needed to add it beacuse the users were losing the loadboard prop produced by other bug and they cannot re link the board again.
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!
I'll take care of #1 and #2.
For #3, the linkedBoard state and linkedBoard tile property are used differently in this component. In state, it contains either a string or a board entity. In the tile property, it is a boolean.
Looking over this again, there are actually two places where we set linkedBoard to NONE_VALUE:
- The existing boards
SELECTfield can passNONE_VALUEtohandleBoardsChange, which will set that aslinkedBoard: https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R555-R583 https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R426-R429 setLinkedBoardStatecan do this if it doesn't find a matching board inboards: https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R344-R355 I'm not sure though why we need the'none'string as opposed to an empty string in these cases. We can leave it as-is, since it seems to work. I was asking in case we want to remove it and just make it''.
Awseome! Yes you are right, it doesn't make sense. We can make it ''
Hi @RodriSanchez1
I have the updates for the propTypes and the first two bullets ready.
It turns out NONE_VALUE is there to make sure the value "None" is shown in the SELECT field if you pick that option:
Without this value it shows an empty field, so we should probably keep it.
Testing Summary
Tested the TileEditor refactor and all functionality is working as expected:
Tested Scenarios
- Adding new tiles (buttons and folders)
- Editing existing tiles (single and multiple)
- Symbol search and image selection
- Custom image uploads
- Background color picker
- Tile type switching (button ↔ folder)
- Board linking functionality
- Multi-tile editing with stepper navigation
- Edge cases (empty values, long labels, rapid clicks)
Results
- No console errors
- All changes persist correctly
- UI remains responsive
- Autofill and search working properly
- Existing boards load and edit without issues
The refactor from class component to function component seems to be working fine and no problems detected with the new changes.