All comments

• 

  Kehinde•660

  @jonathan401Submitted almost 2 years ago

  So it's been a while I worked on any project. This project took me longer than I thought it should have but I'm glad at how it came out. My Javascript code is not really organized so please do well to comment on how I could make the code a little bit cleaner and more readeable. I had quite a challenge connecting up the error messages as well as making sure the custom field was in sync but I was'nt able to do this entirely right. On feedback on how I could fix this is highly appreciated. Thanks for checking out my solution :).

  

  Tip Calculator App

  1

  30
  

  Karol•1,620

  @karolbanatPosted almost 2 years ago

  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

  0

  This comment has received 1 replyShow
• 

  Shha5•70

  @Shha5Submitted almost 2 years ago

  Hello and thank you for taking a look at my solution.

  Please share your feedback! I appreciate it very much.

  I am quite happy with how this solution looks, however there is one thing that I could not figure out so I just left it as it is - when there is no border on the body, a vertical scrollbar appears (and quite a lot of empty body space is added below the footer), despite the fact that all of the contents in theory fit within the viewport height. As soon as I set the body border (it is currently set to be 1px solid transparent) the scrollbar disappears. I know it is happening because I must have done something wrong, but I can't figure out what - so if you know, please tell me so I can avoid this in the future.

  Once again, thank you very much!

  

  Responsive 3-column/rows component using flexbox

  1

  30
  

  Karol•1,620

  @karolbanatPosted almost 2 years ago

  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

  1
• 

  keziarkts•370

  @keziarktsSubmitted almost 2 years ago

  Hi everyone,

  While completing this challenge, the main problem I faced is that for the mobile version, I didn't get the margin around the card (when I usually manage to get this margin). I couldn't figure out why... If anyone can help me with this it would be great!

  Thank you as always 😃

  

  Responsive Single Price Grid - HTML/CSS

  1

  20
  

  Karol•1,620

  @karolbanatPosted almost 2 years ago

  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

  0

  This comment has received 1 replyShow
• 

  Vitor Hugo•60

  @VHAlvesSSubmitted about 2 years ago

  I'm feeling fine with this project, but something stuck in my head, when I use nth child(1) a div I have after the first element is also affected. Could someone explain to me why this happens? ( my solution for this was using id, but i want to know why the value 1 is taking two elements ) I appreciate any and all feedback.

  

  four cards with flexbox

  2

  30
  

  Karol•1,620

  @karolbanatPosted about 2 years ago

  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

  1

  This comment has received 1 replyShow