@danmlarsen
Submitted
What specific areas of your project would you like help with?
I appreciate any helpful feedback.
@mofada
@danmlarsen
Submitted
What specific areas of your project would you like help with?
I appreciate any helpful feedback.
@mofada
Posted
Looks great!
@mofada
Posted
It looks great, congratulations on completing the challenge!
@javiIT10
Submitted
@mofada
Posted
Congratulations on completing the challenge, all is well
@javiIT10
Submitted
@mofada
Posted
First of all, congratulations on completing the challenge.
Let me talk about my opinion on your code, of course, this is only my personal opinion.
First of all, from the page, compared with Solution and Design, your background color seems to be wrong, and some small details are not restored enough.
Secondly, in terms of HTML code, you directly use the H2
tag. Normally, you should start with H1 instead of using H2 directly.
Input and Label are used correctly, but radio group recommends using fieldset
and legend
to implement it.
Then, in terms of input validation, you can use HTML's required
to perform validation and reduce js code.
<input type="text" id="firstName" name="firstName" class="input" required />
Using HTML's own validation can reduce your code
@javiIT10
Submitted
@mofada
Posted
First of all, congratulations on completing the challenge, you are awesome.
Here are some areas I think can be optimized:
1.Details and Summary
I think maybe you use details and summary is better.
Here's an example:
<details class="group" open>
<summary
class="list-none after:w-[30px] after:h-[30px] after:bg-[url('assets/images/icon-plus.svg')] group-open:after:bg-[url('assets/images/icon-minus.svg')]">
What is Frontend Mentor, and how will it help me?
</summary>
<p class="... text style here">
Frontend Mentor offers realistic coding challenges to help developers improve their
frontend coding skills with projects in HTML, CSS, and JavaScript. It's suitable for
all levels and ideal for portfolio building.
</p>
</details>
Here some article will help you:
details
tag.2. Responsive Images
And then, the responsive picture can refer to this Responsive_images
<!--background-->
<picture class="absolute top-0 w-full">
<source media="(min-width: 768px)"
srcset="assets/images/background-pattern-desktop.svg">
<source media="(max-width: 768px)"
srcset="assets/images/background-pattern-mobile.svg">
<img src="assets/images/background-pattern-mobile.svg" alt="Decorative background pattern"
class="w-full object-cover">
</picture>
If you want to known more details, you can check my solutions faq-accordion-with-tailwind-css
@javiIT10
Submitted
@mofada
Posted
I think you can use radio group
replace button
to implement rating
<!--rating-->
<fieldset>
<legend class="sr-only">Rate our service</legend>
<label
class="w-10 h-10 has-[:checked]:bg-white has-[:checked]:text-blue-900 hover:bg-orange hover:text-blue-900 md:w-[50px] md:h-[50px]">
<input type="radio" name="rating" value="1" class="appearance-none" required/>
<span>1</span>
</label>
<label class="w-10 h-10 has-[:checked]:bg-white has-[:checked]:text-blue-900 hover:bg-orange hover:text-blue-900 md:w-[50px] md:h-[50px]">
<input type="radio" name="rating" value="2" class="appearance-none"/>
<span>2</span>
</label>
......
</fieldset>
@javiIT10
Submitted
@mofada
Posted
Your JavaScript code is well-organized and structured. However, there are some suggestions for improving readability, maintainability, and efficiency:
Debounce Input Events: Currently, the event listeners for input fields trigger calculations every time the user types something. To improve performance, especially with large inputs, consider debouncing the input event listeners.
function debounce(func, wait = 300) {
let timeout;
return function (...args) {
clearTimeout(timeout);
timeout = setTimeout(() => func.apply(this, args), wait);
};
}
inputBill.addEventListener("input", debounce(function () {
values.bill = parseFloat(this.value);
if (!isNaN(values.bill)) {
calculate();
}
}));
Consistent Variable Naming: The variable alert
can be confusing since it's a global function in JavaScript. Consider renaming it to something more descriptive, like alertMessage
or alertElement
.
const alertMessage = document.querySelector("#alert");
Separate Calculation Logic: The calculate
function is doing a lot of work. You could separate the logic for calculating the tip amount and the total amount into individual functions. This will make the code easier to test and maintain.
function calculateTipAmount(bill, tip, people) {
return (bill * tip) / 100 / people;
}
function calculateTotalAmount(bill, tip, people) {
return ((bill * tip) / 100 + bill) / people;
}
function calculate() {
const { bill, tip, people } = values;
resultTipValue.innerText = `$${calculateTipAmount(bill, tip, people).toFixed(2)}`;
resultTotalValue.innerText = `$${calculateTotalAmount(bill, tip, people).toFixed(2)}`;
}
Reuse of CSS Classes: In the showAlert
and cleanAlert
functions, you're adding and removing multiple classes for styling. Consider grouping these styles into a single CSS class, like .alert-active
, to reduce redundancy.
.alert-active {
border: 2px solid #CA8073;
}
Then, in your JavaScript:
function showAlert(reference) {
alertMessage.textContent = "Can’t be zero";
reference.classList.add("alert-active");
}
function cleanAlert(reference) {
alertMessage.textContent = "";
reference.classList.remove("alert-active");
}
Handle NaN in calculate
: Ensure that the calculate
function can handle cases where the input values might not be valid numbers (NaN). This can prevent unintended results being displayed.
function calculate() {
const { bill, tip, people } = values;
if (isNaN(bill) || isNaN(tip) || isNaN(people) || people === 0) {
resultTipValue.innerText = `$0.00`;
resultTotalValue.innerText = `$0.00`;
return;
}
resultTipValue.innerText = `$${calculateTipAmount(bill, tip, people).toFixed(2)}`;
resultTotalValue.innerText = `$${calculateTotalAmount(bill, tip, people).toFixed(2)}`;
}
Complete the reset
Function: You have an empty reset
function. Consider moving the reset logic from the btnReset
event listener to this function for better code organization.
function reset() {
values.bill = 0;
values.tip = 5;
values.people = 1;
inputBill.value = 0;
inputCustom.value = 0;
inputPeople.value = 1;
resultTipValue.textContent = "$0.00";
resultTotalValue.textContent = "$0.00";
removeActive();
}
btnReset.addEventListener("click", reset);
Avoid Global Scope Pollution: Consider encapsulating your code within an IIFE (Immediately Invoked Function Expression) to avoid polluting the global scope.
(function() {
// All your code here...
})();
By applying these suggestions, your code will be more efficient, readable, and easier to maintain.
@JohnPugh688
Submitted
What are you most proud of, and what would you do differently next time?
Im happy with my layout on this one, good challenge working with grid, flex and absolute positioning for background images also working with the different screen size adjustments. overall getting the json file to work with Javascript was hard work and once again i dont really understand how i finally got it to work.
What challenges did you encounter, and how did you overcome them?
My biggest struggle with this was fetching the json file. I spent far to long trying to figure out why it would work for me only to finally realise my mistake! BEFORE: const dailyBtn = document.getElementById(".daily"); const weeklyBtn = document.getElementById(".weekly"); const monthlyBtn = document.getElementById(".monthly"); AFTER: const dailyBtn = document.getElementById("daily"); const weeklyBtn = document.getElementById("weekly"); const monthlyBtn = document.getElementById("monthly");
something so simple can cause so much time and effort!!
What specific areas of your project would you like help with?
Im not sure how semantic this ones going to be as i think i might have used divs more than i maybe should have. interested to know if anyone has any better ways to do this to make it easier to understand.
@mofada
Posted
Congratulations on completing the challenge, the hover effect is great.
I can't help you with semantics because I think I have problems with semantics too.
What are you most proud of, and what would you do differently next time?
safelist
in the Tailwind.What challenges did you encounter, and how did you overcome them?
safelist
.What specific areas of your project would you like help with?
I like to write clean code, fast easy and I made lots of ID selectors, may be there is a more efficient way than making a whole bunch of ID selectors.
@mofada
Posted
The HTML code you provided already has a good structure and semantics, but there are still some areas that can be optimized:
div
tags with more semantic tags, such as header
, section
, article
, etc., to improve readability and accessibility.alt
attribute of images: Currently, the alt
attribute of the img
tag is empty, which is not conducive to SEO and barrier-free access. It is recommended to provide descriptive text for each image.lg:hidden
and lg:flex
on image tags may cause unnecessary repeated rendering, so consider unified management.UL
and UL__item
, you can consider more descriptive names to express the purpose more clearly.favicon
icon can consider adding different size versions to meet the needs of different devices.@replayzor
Submitted
What are you most proud of, and what would you do differently next time?
I am proud that i did it
What challenges did you encounter, and how did you overcome them?
No challenge
What specific areas of your project would you like help with?
Anything
@mofada
Posted
First of all, congratulations on completing the challenge. I found some small problems, of course, this is just my own opinion
I think the tailwind color naming should follow lowercase, like this text-color-blue-100
, so you should define it like this
colors: {
'dark-grayish-blue': 'hsl(217, 19%, 35%)',
// or like this
dark:{
grayish:{
blue:'hsl(217, 19%, 35%)'
}
}
}
Then in the desktop you seem to have missed the triangle part, you can use svg icon or use css to draw it, like this:
#tooltip:before {
/*Triangle line*/
position: absolute;
bottom: -9px;
left: 50%;
transform: translateX(-50%);
border-left: 12px solid transparent;
border-right: 12px solid transparent;
border-top: 12px solid hsl(217, 19%, 35%);
}
Finally, your tooltip prompt box seems to have a problem with the position. It does not appear where it should appear. Perhaps you can use js to get the coordinates and display them.
Marked as helpful
@DarrickFauvel
Submitted
What are you most proud of, and what would you do differently next time?
I am proud that outside of Tailwind CSS, I used custom CSS to create responsive grid views for multiple screen widths. Specifically, I used grid template areas to make these responsive layouts.
What challenges did you encounter, and how did you overcome them?
The challenge I encountered was that nearly every testimonial looked different. For this, I targeted each testimonial card using the CSS :nth-child()
selector and applied its specific style.
What specific areas of your project would you like help with?
Did I miss something? Is there anything I can do better? I am open to any constructive feedback.
@mofada
Posted
First of all, congratulations on completing the challenge. It's done very well, awesome!
First of all, from the HTML semantics point of view, it's awesome, I can't fault it. Secondly, you also added hover animation, which is a good attempt. And responsive matching, it's perfect, awesome