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

BookingUnitType config.js variable needed more comments #1317

Merged
merged 4 commits into from
Jun 29, 2020

Conversation

Gnito
Copy link
Contributor

@Gnito Gnito commented Jun 29, 2020

This adds more comments about how to change booking unit type.
I.e. the new API server/proxy creates it a bit more work for people who want to change the unit type from ´line-item/night´ to ´line-item/day´.

We could also pass the bookingUnitType variable among bookingData to client app's server for flexible pricing calculations, but

  1. somehow adding more parameters that affect price calculations feels a bit wrong to me (customizers should think security concerns twice if they add more parameters)
  2. "bookingData" has a bit different meaning in the web app's context (it contains any extra info from BookingDatesForm).

@Gnito Gnito requested a review from OtterleyW June 29, 2020 12:08
const unitType = 'line-item/night';
// This bookingUnitType needs to be one of the following:
// line-item/night, line-item/day or line-item/units
const bookingUnitType = 'line-item/night';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name so that it's in sync with the config.js variable (in web app).

Copy link
Contributor

@OtterleyW OtterleyW left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gnito Gnito merged commit 7f9c8e0 into master Jun 29, 2020
@Gnito Gnito deleted the update-comments-for-bookingUnitType-on-server branch June 29, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants