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

Sizing model JS #47

Merged
merged 3 commits into from
Sep 10, 2020
Merged

Sizing model JS #47

merged 3 commits into from
Sep 10, 2020

Conversation

rviscomi
Copy link
Collaborator

@rviscomi rviscomi commented Sep 8, 2020

Progress on #8

Looking for early feedback to see if I'm using the utils correctly.

Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

Yay, let's get this ball rolling!

  • So for what you're doing here, countDeclarations() would be a better helper, as it returns a total count. Their only difference is that countProperties() returns an object literal of properties to counts, and countDeclarations() just returns a total count. I should rename one or both to make this clearer, suggestions welcome. Maybe countTotalDeclarations() and countDeclarations()? Or countDeclarations() and countDeclarationsByProperty()?
  • If you're just matching by equality you don't need regexes, any test can also be a string, which is faster too, and more accurate, since this matches any part of the value. Unless you were aiming to also match prefixes too? In which case, we'd need a more specific regex, like /^-(o|moz|webkit|ms)-box-sizing$/. In general, whenever possible use ^ and $ anchors for regexes, as it's faster.

@LeaVerou
Copy link
Owner

LeaVerou commented Sep 9, 2020

Alright, I renamed countProperties to countDeclarationsByProperty to make the distinction more clear.

@rviscomi
Copy link
Collaborator Author

SGTM. Could you also update https://projects.verou.me/rework-utils/ with the new function?

@LeaVerou
Copy link
Owner

Done!

Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

Since you only care about 1 count, countDeclarations() is more appropriate here. countDeclarationsByProperty() is for when you're interested in the breakdown per different property. Does that help?

@rviscomi
Copy link
Collaborator Author

Ah ok yes that makes more sense.

Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

I would format the object literal in multiple lines, but that's super minor.

@LeaVerou LeaVerou merged commit e59415e into LeaVerou:master Sep 10, 2020
@rviscomi
Copy link
Collaborator Author

FWIW I borrowed the code from 34-named-grid-lines.js which had it all on one line. 😉

@LeaVerou
Copy link
Owner

Probably because it's shorter, though it would still benefit from some more liberal whitespace :)

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