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

adds leading dash flag feature and corresponding tests #12

Closed
wants to merge 1 commit into from

Conversation

lasagnamassage
Copy link

I'm pretty sure this could be done a bit cleaner, but I saw the opportunity to do a PR and took it. Hope this works :)

Copy link
Owner

@joakimbeng joakimbeng left a comment

Choose a reason for hiding this comment

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

I'm sorry for my late response on this PR! Thanks for your contribution!

I have a few suggestions about the change though ☺️

Comment on lines +35 to +38
test("string reverse without leading dash", (t) => {
const str = "-kebab-case";
t.equal(reverse(str), "KebabCase");
})
Copy link
Owner

Choose a reason for hiding this comment

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

This test doesn't do what it says... According to the test's title the variable should be:

const str = "kebab-case";

But that will give the result:

t.equal(reverse(str), "kebabCase");

Which is not the reverse of what kebabCase outputs with leadingDashFlag=false.
That's my concern about this feature. So that needs to be documented in the readme along with the feature/flag itself.

Copy link
Owner

Choose a reason for hiding this comment

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

There's also a missing newline at the end of the file...

module.exports = exports = function kebabCase(str) {
return str.replace(KEBAB_REGEX, function (match) {
return '-' + match.toLowerCase();
module.exports = exports = function kebabCase(str, leadingDashFlag) {
Copy link
Owner

Choose a reason for hiding this comment

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

"leadingDashFlag" should be named something better. Perhaps "stripLeadingDash"?

Comment on lines +9 to +14
let firstChar = kebab.charAt(1);
return (
leadingDashFlag === false &&
firstChar.toUpperCase() === firstChar
? kebab.slice(1).toLowerCase() : kebab.toLowerCase()
);
Copy link
Owner

Choose a reason for hiding this comment

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

Here you are mixing tabs and spaces...
And you are doing unnecessary calls to toLowerCase() at the end.

What about simply writing the feature like this instead:

module.exports = exports = function kebabCase(str, stripLeadingDash) {
  const kebab = str.replace(KEBAB_REGEX, function (match) {
     return '-' + match.toLowerCase();
   });

  return stripLeadingDash && kebab[0] === '-' ? kebab.slice(1) : kebab;
};

To me that is more readable and more clearly does what it says.

What do you think?

joakimbeng added a commit that referenced this pull request Jun 28, 2024
And a new `keepLeadingDash` option has been added to the kebabCase function.
When set to `false` it will remove any leading dash from the kebab-cased string,
in case the original string starts with a leading uppercase letter.
Fixes #7, Closes #12.

BREAKING CHANGE: kebab-case is rewritten in ESM and is not published as CommonJS anymore. If you still need CJS you should stick to the v1 version.
joakimbeng added a commit that referenced this pull request Jun 28, 2024
And a new `keepLeadingDash` option has been added to the kebabCase function.
When set to `false` it will remove any leading dash from the kebab-cased string,
in case the original string starts with a leading uppercase letter.
Fixes #7, Closes #12.

BREAKING CHANGE: kebab-case is rewritten in ESM and is not published as CommonJS anymore. If you still need CJS you should stick to the v1 version.
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