Skip to content
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

Fixes resizeable columns when the first row has a colspan #4955

Merged
merged 11 commits into from
Oct 4, 2024

Conversation

jaapvanhoek
Copy link
Contributor

@jaapvanhoek jaapvanhoek commented Mar 7, 2024

The width of columns get lost when the first row has a colspan that spans the sized columns because the colwidth (which has a comma separated list of column-widths) gets cast to an int and thus removing the other column-widths

Please describe your changes

The comma separated column-widths for the spanned columns are now split before that are cast to an int.

How did you accomplish your changes

By calling .split on the colwidth variable each width can be parsed separately.

How have you tested your changes

I added the colwidth attribute to the existing example and noticed that it did not behave as expected. Only the first of the spanned columns got the right width. After I changed the code the result was like expected and all columns have the right size, either the given pixel value or it fills the remaining space.

How can we verify your changes

  1. Create a table
  2. Merge the columns/cells of the first row
  3. Resize multiple columns that are spanned by the first row
  4. Use the output HTML as the input for a new table when tiptap loads
  5. The size of each column must be equal to the value that it was given.

Remarks

The issue was with both TableCell and TableHeader

Checklist

  • [v] The changes are not breaking the editor
  • Added tests where possible
  • [v] Followed the guidelines
  • Fixed linting issues

Related issues

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 96939ef
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66ffdc43f151a90008d29813
😎 Deploy Preview https://deploy-preview-4955--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bdbch
bdbch previously approved these changes May 13, 2024
Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

changeset-bot bot commented Jul 4, 2024

🦋 Changeset detected

Latest commit: 96939ef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 54 packages
Name Type
@tiptap/extension-table-cell Patch
@tiptap/extension-table-header Patch
@tiptap/core Patch
@tiptap/extension-blockquote Patch
@tiptap/extension-bold Patch
@tiptap/extension-bubble-menu Patch
@tiptap/extension-bullet-list Patch
@tiptap/extension-character-count Patch
@tiptap/extension-code-block-lowlight Patch
@tiptap/extension-code-block Patch
@tiptap/extension-code Patch
@tiptap/extension-collaboration-cursor Patch
@tiptap/extension-collaboration Patch
@tiptap/extension-color Patch
@tiptap/extension-document Patch
@tiptap/extension-dropcursor Patch
@tiptap/extension-floating-menu Patch
@tiptap/extension-focus Patch
@tiptap/extension-font-family Patch
@tiptap/extension-gapcursor Patch
@tiptap/extension-hard-break Patch
@tiptap/extension-heading Patch
@tiptap/extension-highlight Patch
@tiptap/extension-history Patch
@tiptap/extension-horizontal-rule Patch
@tiptap/extension-image Patch
@tiptap/extension-italic Patch
@tiptap/extension-link Patch
@tiptap/extension-list-item Patch
@tiptap/extension-list-keymap Patch
@tiptap/extension-mention Patch
@tiptap/extension-ordered-list Patch
@tiptap/extension-paragraph Patch
@tiptap/extension-placeholder Patch
@tiptap/extension-strike Patch
@tiptap/extension-subscript Patch
@tiptap/extension-superscript Patch
@tiptap/extension-table-row Patch
@tiptap/extension-table Patch
@tiptap/extension-task-item Patch
@tiptap/extension-task-list Patch
@tiptap/extension-text-align Patch
@tiptap/extension-text-style Patch
@tiptap/extension-text Patch
@tiptap/extension-typography Patch
@tiptap/extension-underline Patch
@tiptap/extension-youtube Patch
@tiptap/html Patch
@tiptap/pm Patch
@tiptap/react Patch
@tiptap/starter-kit Patch
@tiptap/suggestion Patch
@tiptap/vue-2 Patch
@tiptap/vue-3 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jaapvanhoek
Copy link
Contributor Author

Are there any things I could or should do to ensure that this pull request can be merged?

@nperez0111
Copy link
Contributor

@jaapvanhoek I've looked at this before. The thing that worries me is why colwidth would have a comma separated list of values. I haven't looked into whether that is how it works right now or not so maybe you can shed some light on it?

Also seeing more tests would give me higher confidence that it actually achieves the intended behavior without regressions

@jaapvanhoek
Copy link
Contributor Author

jaapvanhoek commented Aug 21, 2024

The thing that worries me is why colwidth would have a comma separated list of values. I haven't looked into whether that is how it works right now or not so maybe you can shed some light on it?

@nperez0111 This is how it currently works when the steps to reproduce are followed, and it has nothing to do with my changes. The only thing I did was change the way the value is handled and make it possible to read the comma-separated values.

Later today I will try add some tests for the table-header extension.

@nperez0111
Copy link
Contributor

@jaapvanhoek thanks for letting me know, I haven't really touched the table extension or read through it to understand it yet. So you can understand why I'm cautious of it. Tests would definitely give me more confidence, so I appreciate it

@jaapvanhoek
Copy link
Contributor Author

After spending some time trying to run the tests, they keep failing at the Cypress step with the error: "Cypress failed to verify that your server is running". Based on what I've found online, this issue might be related to the setup I'm using (MacOS Sonoma 14.6.1), but none of the suggested solutions have worked for me. As a result, I'm currently unable to add tests to the project. Hopefully, someone else can shed some light on why I can't run the tests at all.

@nperez0111
Copy link
Contributor

Hi @jaapvanhoek , so to run the tests you need to have both the development server and the test server running. the development server runs on localhost:3000

So in one terminal you'll need npm run dev and in another, npm run test:open

it will look like this:
image

This opens the cypress UI that allows you to run tests individually in a nide interface rather than only in the terminal which is useful for writing tests.

I hope this is helpful for you

… cells and headers can init with a single and multiple column-widths.
@jaapvanhoek
Copy link
Contributor Author

@nperez0111 I have added the tests you requested, any suggestions?

@SanderLeenders
Copy link
Contributor

@nperez0111 @bdbch can we get an update on this issue?

@nperez0111 nperez0111 changed the base branch from main to develop October 4, 2024 11:52
Copy link
Contributor

@nperez0111 nperez0111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait, @jaapvanhoek really appreciate it, I checked on my side & it is working correctly. Thanks for your contribution!

@nperez0111 nperez0111 merged commit 21df331 into ueberdosis:develop Oct 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants