cboard icon indicating copy to clipboard operation
cboard copied to clipboard

Refactoring TileEditor to function component

Open zkarmi opened this issue 4 months ago • 7 comments

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, and handleTypeChange() can call getDefaultColor() to make it shorter too. Should we refactor this?
  • In the .TileEditor__loadBoard_section div, there is a check for linkedBoard === NONE_VALUE. However, the only time linkedBoard would be NONE_VALUE (which is the string 'none') is if setLinkedBoardState() was called and a matching board could not be found. In all other cases where linkedBoard is nullified, it is set to an empty string (''). Is this intentional?

zkarmi avatar Aug 20 '25 05:08 zkarmi

Hello @tomivm and @RodriSanchez1, can they review this migration? @zkarmi continued with my proposal and finished it.

gonojuarez avatar Aug 20 '25 15:08 gonojuarez

Hi @RodriSanchez1 @tomivm , I wanted to put this back on your radar, as I haven't heard anything in several weeks.

zkarmi avatar Sep 30 '25 14:09 zkarmi

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: image 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 avatar Nov 12 '25 02:11 RodriSanchez1

@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:

  1. The existing boards SELECT field can pass NONE_VALUE to handleBoardsChange, which will set that as linkedBoard: https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R555-R583 https://github.com/cboard-org/cboard/pull/1984/files#diff-93ffb9a636f940f851ec808751d635920889b29ee516010778441ff4a82e49c4R426-R429
  2. setLinkedBoardState can do this if it doesn't find a matching board in boards: 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 ''.

zkarmi avatar Nov 14 '25 16:11 zkarmi

Awseome! Yes you are right, it doesn't make sense. We can make it ''

RodriSanchez1 avatar Nov 14 '25 21:11 RodriSanchez1

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: image Without this value it shows an empty field, so we should probably keep it.

zkarmi avatar Nov 17 '25 19:11 zkarmi

Testing Summary

Tested the TileEditor refactor and all functionality is working as expected:

Tested Scenarios

  1. Adding new tiles (buttons and folders)
  2. Editing existing tiles (single and multiple)
  3. Symbol search and image selection
  4. Custom image uploads
  5. Background color picker
  6. Tile type switching (button ↔ folder)
  7. Board linking functionality
  8. Multi-tile editing with stepper navigation
  9. 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.

magush27 avatar Dec 02 '25 14:12 magush27