Skip to content
OverviewSolutions
73
Comments
4

Karol

@karolbanatPoland1,620 points

Mostly sef-taught, lover of books, interested in Japanese language and culture.

I’m currently learning...

TypeScript, UI/UX Design, AdobeXD

Latest solutions

View more solutions

Latest comments

  • Karol•1,620
    @karolbanat

    Hi @jonathan401, great to see you back 😀 with great solution 🎆✨.

    Here is my feedback:

    • Move the aria-label=custom tip input from li with custom tip input to the input inside
    • The for attribute in <label for="bill">Bill</label> should match that's of input, so change it to bill-field
    • When you type custom tip percent it incorrectly calculates the Total / person amount. To fix it you would probably want to add 1 to customValue in getTotalTips function, in the line where you assign result variable. So just changing (tip * (customValue ? customValue : tipPercent + 1)) to (tip * (customValue ? customValue + 1 : tipPercent + 1)) should fix it.

    Also if I would do that challenge again I would do the tip buttons as custom radio buttons (although i didn't do it that way because I was too lazy 😅).

    And getSingleTip and getTotalTips functions look similar so you could probably refactor it. They get the same parameters sent so maybe you could merge them together, and return an array or object that you would destructure in getResults. And I don't think you need too check for customValue in those two functions as you check for it before sending percentage parameter in getResults function. So if I would do that it would look something like this:

    const getTips = (tip, percentage, totalPersons) => {
	let percent = percentage / 100;
	let tipPerPerson = ((tip * percent) / totalPersons).toFixed(2);
	let totalPerPerson = ((tip * (percent + 1)) / totalPersons).toFixed(2);
	return [tipPerPerson, totalPerPerson];
};

const getResults = (btnId = 0) => {
	customValue = customField.value;
	let [tipValue, totalPersonsValue] = getTips(billField.value, customValue ? customValue : btnId, billers.value);

	tip.textContent = `$${tipValue}`;
	totalTips.textContent = `$${totalPersonsValue}`;
};

    Hope it helps and happy coding 😊.

    Marked as helpful
  • Karol•1,620
    @karolbanat

    Hi @Shha5. Good job completing this challenge 🎉.

    About your question: the space that appears when you remove the border from the body is because of the (vertical) margin you set on .container (15vh in magin: 15vh 24vh 15vh 24vh). It, if I am correct, is called collapsing margins. The border you set on body acts like a 'cushion' for margin on .container and it doesn't 'leave' the body. But if you remove the border, you remove that 'cushion', and then that margin escapes body and is causing body to shift too. You can see that if you change border into outline property and give it colour (for example instead of border: 1px solid transparent set outline: 1px solid firebrick). Outline doesn't count into box model so it won't affect collapsing margins. And because your body has minimal height set to 100vh (min-height: 100vh), it's like body has height of 115vh (100vh (body) + 15vh (leaking margin on top)). And this is the cause of that space [inspect the margins on container in dev tools after setting outline).

    In your case removing min-height from body will remove that problem. But it will still move the body.

    Also I would recommend not relaying on viewport units too much.

    It's a lot to explain so I don't know if i did it right. Hope it helps.

    And here is link to Kevin Powell's video about collapsing margins.

    And once again, congratulations on completing this challenge and happy coding. 😊

    Marked as helpful
  • Karol•1,620
    @karolbanat

    Hi @keziarkts, about your question, the problem probably is that you set fixed width on the .main-card element (width: 700px;). That causes overflow on smaller screens. It is better to not set fixed widths on most of the elements, but if you want to limit the size, you could use max-width. So, here replace width: 700px with max-width: 700px.

    Also in most cases (or probably always) its better to not set fixed height on elements and let content determine used space. So here, also, instead of height: 540px use min-height: 540px or just remove that property from .main-card.

    Hope it helps. Other than that, your solution looks great. ✨ Congratulations and happy coding. 😊

    Marked as helpful
  • Karol•1,620
    @karolbanat

    Hi @VHAlvesS, great work on completing this challenge 🎉. Answering your question: The reason why :nth-child(1) affects two divs is because the first affected div is the first child (nth-child(1)) of the div with class flexWrapper. And the second affected div is also the first child, but of the div with class flexWrapper__mid. So it looks like this. In:

    <div class="flexWrapper">
    <div class="flexWrapper__card">...</div>
    <div class="flexWrapper__mid">...</div>
    <div class="flexWrapper__card">...</div>
</div>

    first <div class="flexWrapper__card">...</div> inside <div class="flexWrapper">...</div> is the first child. And in:

    <div class="flexWrapper__mid">
    <div class="flexWrapper__card" id="builderCard">...</div>
    <div class="flexWrapper__card">...</div>
</div>

    the <div class="flexWrapper__card" id="builderCard">...</div> is the first child of <div class="flexWrapper__mid">.

    Also I see you are using BEM, so you could replace the :nth-child selectors with something like BEM modifier class, for example: <div class="flexWrapper__card flexWrapper__card--cyan">...</div>

    Hope it helps. Have a nice day.

    Marked as helpful