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

Minor cleanup #229

Merged
merged 7 commits into from
Apr 1, 2021
Merged

Minor cleanup #229

merged 7 commits into from
Apr 1, 2021

Conversation

dcousens
Copy link
Collaborator

@dcousens dcousens commented Apr 1, 2021

Firstly, this PR changes the first branch when unpacking object's, back to the primary behavior, with the toString behaviour as an addendum, and fixes some of the mixed indentation that was merged previously.

Secondly, after updating the developer dependencies, I removed the open-cli test target and dependency, as it attempts to open in a compatible program, but squashes any errors.
I don't think opening the default browser is useful when testing compared to maybe providing instructions in the README.

@dcousens dcousens force-pushed the cleanup branch 2 times, most recently from cfceb3f to a6280eb Compare April 1, 2021 03:22
"opn-cli": "4.0.0"
"benchmark": "^2.1.4",
"browserify": "^16.2.3",
"mocha": "^8.3.2"
Copy link
Collaborator Author

@dcousens dcousens Apr 1, 2021

Choose a reason for hiding this comment

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

Updated mocha too, the listed version threw errors on startup for me.

@dcousens
Copy link
Collaborator Author

dcousens commented Apr 1, 2021

@JedWatson tests are passing, but the Travis CI for this repository appears to have disappeared.

@dcousens dcousens requested a review from JedWatson April 1, 2021 03:28
@dcousens dcousens added the dependencies Pull requests that update a dependency file label Apr 1, 2021

export interface ClassArray extends Array<ClassValue> {}

export function classNames(...args: ClassArray): string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the Class prefix (redundant when you are importing).

The mapping of string to truthy values was different between between arrays and objects, which is not what our tests dictate. Changed that.

@@ -8,15 +8,10 @@
// Michal Adamczyk <https://github.com/mradamczyk>
// Marvin Hagemeister <https://github.com/marvinhagemeister>

export type ClassValue = string | number | ClassDictionary | ClassArray | undefined | null | boolean;
export type Value = string | number | boolean | undefined | null;
Copy link
Collaborator Author

@dcousens dcousens Apr 1, 2021

Choose a reason for hiding this comment

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

I don't know if Value is a nice name to be honest.

These types aren't exactly capturing the subtlety of the meaning behind string arguments, compared to truthy values.

@JedWatson JedWatson merged commit 1432ad1 into master Apr 1, 2021
@JedWatson JedWatson deleted the cleanup branch April 1, 2021 05:42
@dcousens
Copy link
Collaborator Author

dcousens commented Apr 1, 2021

For posterity, I benchmarked the 2.3.0 release against 2.2.6 [in Firefox 87] and there was negligible difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants