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

Create package "@eventespresso/config" #1302

Merged
merged 23 commits into from
May 2, 2024
Merged

Conversation

alexkuc
Copy link
Contributor

@alexkuc alexkuc commented May 1, 2024

Wait for PR #1298 to be merged

Fix #1299

@github-actions github-actions bot removed C: accessibility ♿ category C: UI/UX 🚽 category D: Event Smart 🛒 domain: Event Smart labels May 1, 2024
@alexkuc
Copy link
Contributor Author

alexkuc commented May 1, 2024

Rebased and force-pushed due to merging of #1298 into trunk

@alexkuc
Copy link
Contributor Author

alexkuc commented May 1, 2024

Current local commit sha is 3846093. Going to test it:

  • reinstall node_modules ✅
  • linter ✅
  • build ✅
  • unit tests ✅
  • e2e tests ✅

// TODO: consolidate data types
// LATER: consolidate data types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a separate GH issue now for this and related LATERs - #1301

@@ -1,5 +1,6 @@
module.exports = [
/* LEVEL 0 */ ['types'],
/* LEVEL -1 */ ['types'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basement!

Copy link
Member

Choose a reason for hiding this comment

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

FYI... those indexes are just for display purposes... you could have just changed them to go from 1 - 10, or 1 - 11, or whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured that. Didn't want to introduce unnecessary change to PR hence -1.

decimalPlaces: 2,
decimalMark: '.',
thousandsSeparator: ',',
subunits: 100, // Math.pow(10, 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explanation at how we arrive at number 100

Copy link
Member

Choose a reason for hiding this comment

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

right, based on the decimal places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My interpretation of the default value came from here:

subunits: config?.subunits >= 0 ? config.subunits : Math.pow(10, config?.decimalPlaces || 2),

By default value, I mean when the config and all subsequent properties are undefined.

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

AWESOME !!! 🍎 👏🏻

@@ -1,5 +1,6 @@
module.exports = [
/* LEVEL 0 */ ['types'],
/* LEVEL -1 */ ['types'],
Copy link
Member

Choose a reason for hiding this comment

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

FYI... those indexes are just for display purposes... you could have just changed them to go from 1 - 10, or 1 - 11, or whatever

decimalPlaces: 2,
decimalMark: '.',
thousandsSeparator: ',',
subunits: 100, // Math.pow(10, 2);
Copy link
Member

Choose a reason for hiding this comment

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

right, based on the decimal places

@alexkuc
Copy link
Contributor Author

alexkuc commented May 2, 2024

Re-tested with commit sha 49162e8 locally:

  • linter ✅
  • builder ✅
  • unit tests ✅
  • e2e tests ✅

@alexkuc alexkuc merged commit 9dd6785 into master May 2, 2024
10 checks passed
@alexkuc alexkuc deleted the MOD/create-package-config branch May 2, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: automation & deployment ⚙️ category C: build process 🔨 category C: data systems 🗑 category C: services 🤝 category D: EE CORE ☕ domain: Event Espresso Core Plugin D: Packages 📦 domain: Barista Packages D: WP User Add-on 👤 domain: WP User Integration add-on S11: completed 🚀 status
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create package "config"
7 participants