Skip to content

Fully designed the calculator - Adir Avraham.#13

Open
GilHeller wants to merge 3 commits intomainfrom
adir-calculator
Open

Fully designed the calculator - Adir Avraham.#13
GilHeller wants to merge 3 commits intomainfrom
adir-calculator

Conversation

@GilHeller
Copy link
Copy Markdown
Contributor

No description provided.

@OptimaLPro OptimaLPro self-requested a review December 5, 2024 13:36

.form__number-input {
padding: 6px 12px;
font-size: 20px;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because the input is Number, make sure the 2 up\down arrow buttons are not under the Dollar ican or under the Icon the Number of Peoeple.
You can use more from the padding to achive this:

padding: 6px 12px 6px 30px;

It will put the arrow buttons on the left of the icon.

Comment on lines +70 to +71
<div class="inputs-section__tip-section--tip-selection-group">
<button class="btn" value="5">5%</button>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good technique of Naming conventions in class names,
but try to avoid such a long names.

index.js Outdated
});

const onCustomPercentageChange = (e) => {
percentageValue = e.target.value;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Make sure to decalre it as a const variable.
Think maybe to parse it, to check if its a number.
currently, if i put "abc" (letters and not numbers), i get Nan (not a number) error in the tip display box.

const percentageValue = parseFloat(e.target.value);

} else {
toggleInputError(e.target, numberOfPeopleErrorMessage, "");
numberOfPeople = Number(numberOfPeople);
calculateTip();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't see the tipping per person in the calculation. i only get the calculation for one person.

i thing you should add

numberOfPeople = Number(numberOfPeopleValue); (and not for Number(numberOfPeople))

Something doesn't work here, check this please

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for the code review.

There's something I didn't understand.

What should I show on the summary list?

Tip amount - Is it the total tip? or how much each person tips?
Toal - is it the total price, including tips? Or the total tip?

image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tip amount = how much each person tips
Total = is it the total price, including tips, for each person.

Comment on lines +73 to +75
percentageButtons.forEach((button) => {
if (button.value === e.target.value) selectedPercentage = button;
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good using of forEach

Comment on lines +2 to +18
const percentageButtons = document.querySelectorAll(
".inputs-section__tip-section--tip-selection-group button"
);
const customPercentageInput = document.querySelector(
"#custom-percentage-input"
);
const fifteenPercentageButton = document.querySelector(
'.inputs-section__tip-section--tip-selection-group button[value="15"]'
);
const billInput = document.querySelector("#bill-input");
const numberOfPeopleInput = document.querySelector("#number-of-people-input");
const tipAmountLabel = document.querySelector("#tip-amount");
const totalPriceLabel = document.querySelector("#total-price");
const billInputErrorMessage = document.querySelector("#bill-error-message");
const numberOfPeopleErrorMessage = document.querySelector(
"#number-of-people-error-message"
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good declaration of variables.
Clear and understandable!

<p class="list-item__labels-group-header">Tip Amount</p>
<p class="list-item__labels-group-caption">/ person</p>
</div>
<p class="list-item__total-price" id="tip-amount">$4.27</p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Make sure text is 0.00 in the init of the site

Comment on lines +6 to +15
.calculator-box {
width: 100%;
margin: 50px 0 0 0;
border-bottom-left-radius: 0;
border-bottom-right-radius: 0;
flex-direction: column;
align-items: center;
gap: 32px;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good responsive method

Copy link
Copy Markdown

@OptimaLPro OptimaLPro left a comment

Choose a reason for hiding this comment

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

High quality code,
good job

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.

3 participants