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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jaapvanhoek
Copy link

@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 c073062
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66eaa01eaac37d00084926b0
😎 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

⚠️ No Changeset found

Latest commit: c073062

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@jaapvanhoek
Copy link
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
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
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
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to Merge
Development

Successfully merging this pull request may close these issues.

3 participants