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

Fix bug not being able to disable H1 #247

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Conversation

mortmoe
Copy link
Contributor

@mortmoe mortmoe commented Jul 18, 2024

Suggested fix for #228

Copy link
Owner

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @mortmoe! Generally looks good to me! Would you be able to squash your commits and remove changes to pnpm-lock? No changes to the lockfile should be necessary for this small tweak in MenuSelectHeading.tsx.

@@ -200,6 +194,15 @@ export default function MenuSelectHeading({
return new Set(headingExtension?.options.levels ?? []);
}, [editor]);

// We have to pass a level when running `can`, so this is just an arbitrary
// one. And we have to check `currentLevel` to prevent all other heading
Copy link
Owner

Choose a reason for hiding this comment

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

minor: say "an arbitrary one that is enabled" instead now

// https://github.com/sjdemartini/mui-tiptap/issues/197).
const canSetHeading =
enabledHeadingLevels.size > 0 &&
(currentLevel === [...enabledHeadingLevels][0] ||
Copy link
Owner

Choose a reason for hiding this comment

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

To make this more performant, can you pull out [...enabledHeadingLevels][0] into a local variable instead of repeating it twice? And for getting the value, can you update this to use enabledHeadingLevels.values().next()? The list spread approach is more expensive (e.g. see here for an explanation)

@mortmoe
Copy link
Contributor Author

mortmoe commented Jul 18, 2024

I'll do my best and try to fix this tomorrow. I'm a bit of a noob when it comes to working with GitHub, kind of my first time doing something like this 🤣 Googling like a maniac on how to get this tested locally, use Forks and all that 🤣 Fun to learn new stuff and probably long overdue anyway so I'll figure it out 👍

@mortmoe mortmoe marked this pull request as draft July 18, 2024 20:46
Build

Removed build in package json
@mortmoe
Copy link
Contributor Author

mortmoe commented Jul 19, 2024

I think I've got it now. As I said, kind of new to this... I've tried several methods for using and testing this in my application:

  1. By linking to my local computer: "yarn add link:/home/morten/PhpstormProjects/mui-tiptap". In this case I run npm run build in this project, and it seems to work but I get a runtime error, something is wrong with useState. Tried just adding a symlink in node_modules as well but same result.
  2. Installing from my fork: "yarn add mui-tiptap@https://github.com/mortmoe/mui-tiptap.git". It gets installed but no dist folder in the node_modules/mui-tiptap ... so it fails to get the "dist/esm/index". When installing from your original I see the dist folder.
  3. Adding "npm run build" to the prepare script in package.json and pushing to the fork fixes 2:
    npm run build && husky install
    However, this fails on my projects gitlab ... wrong node version, so I need to wait util the right guy returns from vacation before I can fix that and publish a working version for my test-users.

I'm probably missing something obvious in 1. and 2. right? :)

Anyway, I've tested the changes using method 3 and removed the build stuff now :)

@mortmoe mortmoe marked this pull request as ready for review July 19, 2024 15:36
@sjdemartini
Copy link
Owner

Thanks for cleaning up the commits and working through this! I'll make a couple tiny tweaks to the comments and variable definitions and merge this soon! 😄

In terms of testing out your change, I would recommend just testing with pnpm dev (see https://github.com/sjdemartini/mui-tiptap/blob/main/CONTRIBUTING.md#development-setup) and changing useExtensions to use HeadingWithAnchor.configure({ levels: [2,3,4] }) or whatever headings you like here

HeadingWithAnchor,
.

If you want to test within your own project (which shouldn't be necessary for this specific bug fix, but maybe useful otherwise), I've found https://github.com/wclr/yalc to be a convenient option.

@sjdemartini sjdemartini enabled auto-merge (squash) July 22, 2024 18:07
@sjdemartini sjdemartini merged commit d312748 into sjdemartini:main Jul 22, 2024
1 check passed
@mortmoe
Copy link
Contributor Author

mortmoe commented Jul 22, 2024

Cool. Thanks for this, and thanks for the tips :) I did some testing with dev actually, just wanted to get it working locally to find a way to do it for other cases later on. I'll look into yalc next time :)

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

Successfully merging this pull request may close these issues.

2 participants