Skip to content

Conversation

hyperz111
Copy link
Contributor

@hyperz111 hyperz111 commented Aug 13, 2025

In this PR, i fix note component overflow bug when use long text. This mean note component will wrap your text based on your terminal width.

Before:
Screenshot_2025_0813_145751.png

After:
Screenshot_2025_0813_145610.png

Copy link

changeset-bot bot commented Aug 13, 2025

🦋 Changeset detected

Latest commit: 8e68b31

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

This PR includes changesets to release 1 package
Name Type
@clack/prompts 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

Copy link

pkg-pr-new bot commented Aug 13, 2025

@example/basic@example/changesets

npm i https://pkg.pr.new/bombshell-dev/clack/@clack/core@376
npm i https://pkg.pr.new/bombshell-dev/clack/@clack/prompts@376

commit: 8e68b31

@hyperz111
Copy link
Contributor Author

hyperz111 commented Aug 13, 2025

Wait, i have a problem...

@hyperz111
Copy link
Contributor Author

Ok, done

@hyperz111
Copy link
Contributor Author

Hello everyone, this is good?

import { note } from "@clack/prompts";
import color from "picocolors";

const longText = "Incididunt dolor sunt aliqua minim labore tempor ea ea esse aliquip. Laborum ipsum qui velit duis ullamco minim amet eu amet dolore. Eiusmod fugiat quis laboris id occaecat velit anim laboris ullamco exercitation sint est.";

await note(longText, "title", {
  format: (line) => color.dim(`* ${line} *`)
});

Screenshot_2025_0814_083230.png

@hyperz111
Copy link
Contributor Author

@43081j, @dreyfus92

Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, LGTM 🤘🏻

@hyperz111
Copy link
Contributor Author

@43081j, can you merge this?

const format = opts?.format ?? defaultNoteFormatter;
const wrapMsg = wrapWithFormat(message, output.columns - 6, format);
const lines = ['', ...wrapMsg.split('\n').map(format), ''];
const titleLen = strip(title).length;
Copy link
Collaborator

@43081j 43081j Aug 22, 2025

Choose a reason for hiding this comment

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

the thing that makes this harder to review is stuff like this where the consts have been reordered unnecessarily

for future reference, diffs should be as concise as possible to make for easy reviewing. so its a good idea to avoid doing this

similarly, the reduce change further down doesn't improve anything, it does the same job a different way and adds to the diff

@43081j
Copy link
Collaborator

43081j commented Aug 22, 2025

Sorry it's taken me so long to review this

I'll get to it this weekend if I can 👍

@hyperz111
Copy link
Contributor Author

hyperz111 commented Aug 24, 2025

@43081j, i have do minimal changes. Can you review it?

Copy link
Collaborator

@43081j 43081j left a comment

Choose a reason for hiding this comment

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

this looks good to me. thanks so much for simplifying the diff too

@43081j 43081j merged commit 9999adf into bombshell-dev:main Aug 25, 2025
7 checks passed
@hyperz111 hyperz111 deleted the fix-note-overflow branch August 25, 2025 10:54
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.

3 participants