feat(react): Added select component for text align feature#2728
feat(react): Added select component for text align feature#2728GuiLeme wants to merge 7 commits into
Conversation
|
@GuiLeme is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a TextAlignSelect formatting toolbar dropdown, updates many locale files to include user-facing alignment ChangesText Alignment Toolbar Component
Sequence Diagram(s)sequenceDiagram
participant User
participant FormattingToolbar
participant TextAlignSelect
participant Editor
participant Storage
User->>FormattingToolbar: open select
FormattingToolbar->>TextAlignSelect: render options (localized labels)
User->>TextAlignSelect: choose alignment
TextAlignSelect->>Editor: focus, read selection
alt selection is table cells
TextAlignSelect->>Editor: update table block content (table cells props)
else normal blocks
TextAlignSelect->>Editor: update selected blocks' props.textAlignment
end
Editor->>Storage: persist changes
Editor-->>User: cursor restored, UI updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/i18n/locales/pt.ts`:
- Around line 318-333: The 21 locale files are missing the required label
properties for the alignment entries causing a type mismatch against Dictionary
= typeof en; update each locale where align_left, align_center, align_right, and
align_justify exist but lack a label by adding a label field (matching the
intent of the tooltip or using the same short word used in en/pt) to each of
those keys (align_left, align_center, align_right, align_justify) so every
locale object satisfies the Dictionary shape; ensure the new label fields are
string values consistent with the locale language.
In
`@packages/react/src/components/FormattingToolbar/DefaultSelects/TextAlignSelect.tsx`:
- Around line 75-93: The table branch can crash and shows wrong alignment
because it unconditionally reads (firstBlock.content as
TableContent).rows[0].cells[0] and ignores the actual cell selection; update the
block in the TextAlignSelect logic to (1) check that firstBlock.content is a
TableContent and that rows exists and rows.length>0 and the targeted row has
cells and cells.length>0, returning undefined if bounds fail, and (2) use the
TableHandlesExtension.getCellSelection() result to determine the selected cell
index/coordinates (use the first selected cell if multiple) instead of
hardcoding [0][0], then pass that specific cell into mapTableCell to read
props.textAlignment; keep existing guards (selectedBlocks.length === 1,
blockHasType, etc.) and return the same shape ({ textAlignment, blocks:
[firstBlock]}) when valid.
- Around line 127-136: The current code shallow-copies table cells via
mapTableCell so the nested props object is still the original reference; then
newTable[row].cells[col].props.textAlignment = textAlignment mutates the live
block state before updateBlock is called. Fix by replacing the targeted cell
with an updated copy that deep-copies props (e.g., set newTable[row].cells[col]
= { ...newTable[row].cells[col], props: { ...newTable[row].cells[col].props,
textAlignment } }) so you never mutate the original props object; update the
logic around mapTableCell/newTable/cellSelection to assign the new cell object
rather than mutating props in-place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fda086e2-5f8f-4b85-97a9-86f5878295a8
📒 Files selected for processing (4)
packages/core/src/i18n/locales/en.tspackages/core/src/i18n/locales/pt.tspackages/react/src/components/FormattingToolbar/DefaultSelects/TextAlignSelect.tsxpackages/react/src/index.ts
|
I think the floating toolbar would also benefit from this new component! Of course, that would be for a future PR. I'm just sharing feedback as someone who has been using BlockNote every day |
|
I'll let @matthewlipski lead here, but I'm generally for this change. I probably wouldn't even have "Left" in the toolbar just the icon perhaps with the existing tooltip |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/unit/react/TextAlignSelect.test.tsx (1)
21-87: ⚡ Quick winMake cleanup unconditional in tests to avoid leaked editor/root state.
On failure paths, cleanup at Line 36-38 / Line 54-56 / Line 85-87 is skipped. Move cleanup toafterEach-managed refs or wrap each test body intry/finally.♻️ Suggested pattern
describe("TextAlignSelect", () => { let container: HTMLDivElement; + let root: ReturnType<typeof createRoot> | undefined; + let editor: BlockNoteEditor | undefined; beforeEach(() => { container = document.createElement("div"); document.body.appendChild(container); }); afterEach(() => { + root?.unmount(); + editor?._tiptapEditor.destroy(); + root = undefined; + editor = undefined; document.body.removeChild(container); }); it("renders null when editor is not editable", () => { - const editor = BlockNoteEditor.create(); + editor = BlockNoteEditor.create(); editor.isEditable = false; - const root = createRoot(container); + root = createRoot(container); @@ - root.unmount(); - editor._tiptapEditor.destroy(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/unit/react/TextAlignSelect.test.tsx` around lines 21 - 87, Tests currently skip cleanup on failure because root.unmount() and editor._tiptapEditor.destroy() are called inline; make cleanup unconditional by moving root and editor refs to outer-scope variables and adding an afterEach that unmounts the root (if set) and destroys the editor._tiptapEditor (if set), or wrap each test body in try/finally to call root.unmount() and editor._tiptapEditor.destroy(); reference BlockNoteEditor.create(), createRoot(container), the root variable, and editor._tiptapEditor.destroy() when implementing the unconditional cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/i18n/locales/zh.ts`:
- Around line 379-382: The tooltip for align_justify is too generic ("文本对齐") and
should match the label "两端对齐"; update the align_justify object so its tooltip
property is the same descriptive text as label (e.g., change tooltip to "两端对齐")
to avoid ambiguity when displayed in the UI.
---
Nitpick comments:
In `@tests/src/unit/react/TextAlignSelect.test.tsx`:
- Around line 21-87: Tests currently skip cleanup on failure because
root.unmount() and editor._tiptapEditor.destroy() are called inline; make
cleanup unconditional by moving root and editor refs to outer-scope variables
and adding an afterEach that unmounts the root (if set) and destroys the
editor._tiptapEditor (if set), or wrap each test body in try/finally to call
root.unmount() and editor._tiptapEditor.destroy(); reference
BlockNoteEditor.create(), createRoot(container), the root variable, and
editor._tiptapEditor.destroy() when implementing the unconditional cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3ec5dd1-040a-45d7-ac15-49343178b42d
📒 Files selected for processing (23)
docs/content/docs/react/components/formatting-toolbar.mdxpackages/core/src/i18n/locales/ar.tspackages/core/src/i18n/locales/de.tspackages/core/src/i18n/locales/es.tspackages/core/src/i18n/locales/fa.tspackages/core/src/i18n/locales/fr.tspackages/core/src/i18n/locales/he.tspackages/core/src/i18n/locales/hr.tspackages/core/src/i18n/locales/is.tspackages/core/src/i18n/locales/it.tspackages/core/src/i18n/locales/ja.tspackages/core/src/i18n/locales/ko.tspackages/core/src/i18n/locales/nl.tspackages/core/src/i18n/locales/no.tspackages/core/src/i18n/locales/pl.tspackages/core/src/i18n/locales/ru.tspackages/core/src/i18n/locales/sk.tspackages/core/src/i18n/locales/uk.tspackages/core/src/i18n/locales/uz.tspackages/core/src/i18n/locales/vi.tspackages/core/src/i18n/locales/zh-tw.tspackages/core/src/i18n/locales/zh.tstests/src/unit/react/TextAlignSelect.test.tsx
✅ Files skipped from review due to trivial changes (7)
- packages/core/src/i18n/locales/vi.ts
- packages/core/src/i18n/locales/he.ts
- packages/core/src/i18n/locales/fr.ts
- docs/content/docs/react/components/formatting-toolbar.mdx
- packages/core/src/i18n/locales/es.ts
- packages/core/src/i18n/locales/no.ts
- packages/core/src/i18n/locales/fa.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react/src/components/FormattingToolbar/DefaultSelects/TextAlignSelect.tsx`:
- Around line 140-148: The write loop in TextAlignSelect that iterates
cellSelection.cells can access undefined rows/cells on newTable and throw;
update the loop in the handler that builds newTable to mirror the read-path
guards (like rows[row]?.cells[col]) by checking that newTable[row] and
newTable[row].cells[col] exist before assigning or mutating; for example, skip
or continue when those are undefined (or create shallow copies as needed) so
assignments to newTable[row].cells[col].props.textAlignment are safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6ee18f4-70fe-4e5a-a685-fd91ce9bb3f3
📒 Files selected for processing (1)
packages/react/src/components/FormattingToolbar/DefaultSelects/TextAlignSelect.tsx
|
Hello, @nperez0111. Thanks for the feedback. I agree with removing the text from the selected Item, but the problem is:
So for that, the best approach (we thought of) would be to also change the If that's implemented, we could use that flag in the What do you say? |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I understand that the I'm proposing that the change here should be:
I'll let @matthewlipski lead here though |
4 participants
Summary
Added a selector component (Just like we have for the block-type selector) for the text align feature.
Rationale
I am thinking of 2 good reasons for this addition:
Besides those, there is one other reason.
With that in mind, it would be much simpler if we had this component for us to use it out-of-the-box.
Changes
TextAlignSelect.tsxcomponent;Impact
As this will not be part of the default toolbar, I'd say no impact on the other components (it will just be used by developers to create their own customized toolbar).
Testing
I didn't create any automated tests, but if you guys wanted to have something like that, I can dig further.
As for manual testing, I just changed one of the playground examples and tried to break it some how.
In my case, I used
examples/03-ui-components/02-formatting-toolbar-buttons/src/App.tsxI added it in place of the three align buttons:After that I just had to build the lib and test it locally.
Screenshots/Video
Checklist
I'll definitely dig into all that for the following commits.All of the above is covered now.Additional Notes
I'd just like to mention that the Logic I use for the align text in the selector is exactly the same as for the button. So that's why I say it follows the project coding standards.
PR I did the same thing for BBB is here
Summary by CodeRabbit
New Features
Localization
Documentation
Tests