@samoina
Latest solutions
Latest comments
- P@jacksen30
Hello @samoina,
Congratulations on completing this challenge! You've done a fantastic job. The image overlay on hover makes this task more intricate than it initially seems.
I noticed you submitted your solution a while ago without receiving any feedback. It's unfortunate to see a decline in the number of people offering feedback and advice in this forum.
Turning to your code, a minor discrepancy caught my eye: the child elements within .main__avatar aren't flush with the left edge of the card, unlike the other elements. Since you've already configured it with display: flex, rectifying this is straightforward. Simply specify width: 100% to ensure this div mirrors the size of its parent component, excluding the padding.
Additionally, there seems to be a tad too much padding on the card. Reducing the padding should bring it closer to the intended design:
.main { padding: 1.2rem; }Something more to think about - CSS Variables Should Convey Purpose:
Adopting meaningful names for CSS variables not only adds code clarity but also fosters design flexibility and better collaboration. Naming conventions based on appearance, like --main--very-dark-blue, don't convey their intended application or adjust gracefully with design alterations. Context-based naming, for example, --primary-background, offers more intuitive understanding and provides flexibility in Theming if you decide to change your design theme later, and the primary background color is updated.
I, too, often overlook this aspect, but it's a valuable point to bear in mind for the future.
I hope you find these suggestions helpful! Let me know if you have any questions or if there's anything else I can assist with. Keep up the great work ! 🚀🌟😊
- @samoinaP@jacksen30
Hello @samoina,
Firstly, great job on the code! It's evident that you've put great effort and thought into this, it really shows. 😃
I had to have a really good look to find where I could offer some constructive and helpful feedback. Below are a couple of areas I found that could benefit from some tweaks:
Button Hover Issue:
There seems to be a layout shift when hovering over the button due to the border being added. A simple fix is to move or include the same border width in the button's base state to maintain size consistency.
Original Code:
button { font: inherit; padding: 1rem; border: none; border-radius: 3rem; background-color: var(--very-light-gray); } button:hover { border: 2px solid white; cursor: pointer; color: var(--very-light-gray); }Refactored To Fix layout height change on button:hover :
button { font: inherit; padding: 0.5rem 1rem; /* Similar To The Design */ border: 2px solid white; /* Moved from :hover to base state */ border-radius: 3rem; background-color: var(--very-light-gray); } button:hover { cursor: pointer; color: var(--very-light-gray); background-color: inherit; }CSS Refactoring for Button Hover Background Color:
Rather than setting individual background colors for each button on hover, it's possible to streamline the code with inheritance. This can help reduce redundancy.
Original Code:
.button__sedans:hover { background-color: var(--bright-orange); } .button__suvs:hover { background-color: var(--dark-cyan); } .button__luxury:hover { background-color: var(--very-dark-cyan); }Can be refactored down to:
.button:hover { background-color: inherit; }Again, well done on the coding. These are just some suggestions to further refine it. Looking forward to seeing more of your work in the future! 💻
- @leodk293P@jacksen30
Hey @leodk293,
Good effort! The solution definitely requires a bit more fine-tuning, though. I'd recommend starting off with some of the more basic challenges first. Here are some points to consider:
HTML Structure and Nesting:
The <main> tag must be nested within the <body> tag. Currently, it's outside of it. Remember, all the content of the page goes inside the <body> tag.
Use of Headings:
Instead of using generic div tags for the titles like 'SEDANS', 'SUVS', and 'LUXURY', consider using semantic heading tags. This could be <h1>, <h2>, or <h3>, depending on the content hierarchy.
Semantic Tags:
Utilize semantic HTML tags where appropriate. This enhances both accessibility and the meaning of the content. For instance, the description of each car type can be wrapped in a <p> (paragraph) tag. Additionally, if "Learn More" is intended to be a link, you should use the <a> tag.
Line Breaks:
Be cautious with the use of <br> tags for content formatting. They should be used sparingly. Instead, try to control content presentation and spacing using CSS.
I've noticed a lot of redundancy with your CSS code, when multiple classes or elements require the same property and values it best to create another generic class for that element for example="container1 container",
please note below I only consider reducing redundancy not fixing the multiple other issues that should be addressed.
Here is your original code:
body{ display: flex; justify-content: center; } .overall{ display: flex; flex-direction: row; } .container1{ padding-left: 20px; height: 400px; width: 300px; display: flex; justify-content: space-around; flex-direction: column; border: 1px solid orange; background-color: orange; } .container2{ padding-left: 20px; height: 400px; width: 300px; display: flex; justify-content: space-around; flex-direction: column; border: 1px solid #207178; background-color: #207178; } .container3{ padding-left: 20px; height: 400px; width: 300px; display: flex; justify-content: space-around; flex-direction: column; border: 1px solid #0A5054; background-color: #0A5054; } .item1,.item4, .item7{ color: #EFEEEE; font-size: 30px; } .item3{ text-align: center; width: 130px; height: 35px; border: 1px solid white; background-color: white; color: orange; border-radius: 15px; } .item6{ text-align: center; width: 130px; height: 35px; border: 1px solid white; background-color: white; color: #207178; border-radius: 15px; } .item9{ text-align: center; width: 130px; height: 35px; border: 1px solid white; background-color: white; color: #0A5054; border-radius: 15px; } a{ font-size: 30px; } .item2,.item5, .item8{ color: #ECEBEB; } @media screen and (max-width:990px){ .overall{ flex-direction: column; height: auto; } }Here is a refactored version that moves a lot of the container and item properties and values to a generic class as to avoid unnecessary duplication of CSS rules:
body { display: flex; justify-content: center; } .overall { display: flex; flex-direction: row; } .container { padding-left: 20px; height: 400px; width: 300px; display: flex; justify-content: space-around; flex-direction: column; } .container1 { background-color: orange; } .container2 { background-color: #207178; } .container3 { background-color: #0A5054; } .item1,.item4, .item7 { color: #EFEEEE; font-size: 30px; } .item { text-align: center; width: 130px; height: 35px; border: 1px solid white; background-color: white; border-radius: 15px; } .item3 { color: orange; } .item6 { color: #207178; } .item9 { color: #0A5054; } a { font-size: 30px; } .item2,.item5, .item8{ color: #ECEBEB; } @media screen and (max-width:990px){ .overall{ flex-direction: column; height: auto; } }If you want to start on some of the more simple challenges I'll be more than happy to provide more detailed feedback on specific pieces of code to help you improve, as you work up to the more difficult challenges. I hope you find my consideration points helpful ☺️
- @MtalafaP@jacksen30
Hey @Mtalafa great work on keeping up the consistency on submitting solutions, the code for this looks super clean congratulations ! Getting to good to quick I wont have any feedback to give soon 😃
Here are a couple of points to think about:
CSS Variables Should Convey Purpose:
Using meaningful names for CSS variables enhances code clarity, supports design flexibility, and improves collaboration by conveying purpose rather than just appearance. Names like --cyan don't indicate their use or adapt well to design changes. A context-based naming convention, such as --primary-background, is more intuitive and maintainable. This provides flexibility in Theming if you decide to change your design theme later, and the primary background color shifts from cyan to magenta, then the variable name --cyan no longer accurately represents its value, leading to potential confusion.
I noticed an opportunity to slightly refactor the button styles and eliminate some redundancy:
Redundant Styles: The background-color: var(--bright-yellow); in the button:hover selector is redundant since it is the same as the default button style. The transition property in the :hover state is also redundant, as transitions are typically defined on the base state and are automatically applied during the state change. So we can safely remove this.
Transition Specification: While not strictly necessary, it's more efficient to specify which properties will transition rather than using transition: all 0.3s;. I've adjusted it to transition: background-color 0.3s, filter 0.3s; to target only the properties that change on hover.
Original CSS
button { color: var(--white); background-color: var(--bright-yellow); font-size: 1.5rem; font-weight: bold; letter-spacing: 0.5px; padding: 1.5rem; margin-bottom: 1rem; border: none; border-radius: 5px; transition: all 0.3s; } button:hover { cursor: pointer; background-color: var(--bright-yellow); filter: brightness(90%); transition: all 0.3s; }Refactored To avoid redundency and to be more specific about the transition that takes place on a hover event
button { color: var(--white); background-color: var(--bright-yellow); font-size: 1.5rem; font-weight: bold; letter-spacing: 0.5px; padding: 1.5rem; margin-bottom: 1rem; border: none; border-radius: 5px; transition: background-color 0.3s, filter 0.3s; } button:hover { cursor: pointer; filter: brightness(90%); }These are crucial considerations. Admittedly, both in the past and during hurried moments now, I've occasionally overlooked these practices myself. Nonetheless, I wanted to share these insights with you, hoping to offer something valuable for your learning journey. It's unfortunate that there seems to be a current lack of advice or feedback on peoples solutions, but I'm here to support you in any way I can.
Marked as helpful - @Wael-OrrabyP@jacksen30
Hello @Wael-Orraby, Good work getting the Javascript Functionality working well !
I've got a small recommendation on the CSS, for a more consistent user experience, particularly regarding the resizing issue you're encountering with new quotes, consider the following modifications:
.container Width Adjustment:
On smaller screens, it might be beneficial to set the .container width to 90%. For larger screens, a fixed width, say 500px, can prevent the container from resizing every time a new quote is fetched. This ensures a consistent width across different quotes, enhancing user experience. However, keeping the height flexible is a good call, allowing it to adjust based on the quote's length.
Body Height:
Also I'd recommend setting the body height to 100vh. Currently, it's set to 96vh, but using the full viewport height ensures that the layout utilizes the entire visible screen area.
I hope these suggestions enhance the responsiveness and appearance of your design. Let me know if you'd like any further clarifications or feedback.
Marked as helpful - @olena-ostapenkoP@jacksen30
Hello @olena-ostapenko,
Your solution looks great,
A couple of small suggestions I have are, Instead of using an anchor tag for the button, it would be more semantically correct to use a button element. For instance:
<button class="decor-circle" aria-label="Update Quote"> <img class="dice" src="./images/icon-dice.svg" alt=""> </button>For the animation it could be simplified in the CSS like so:
/* Defines the rotation animation */ @keyframes rotate { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } }It might be beneficial to initiate the rotation at the beginning of your function and halt it upon either the successful retrieval of data or in the event of an error. This adjustment ensures the animation runs only while data is being fetched, rather than for a predetermined duration. This was the solution that I came to:
function retriveQuote () { // Start the rotation animation button.style.animation = "rotate 1s linear infinite"; fetch('https://api.adviceslip.com/advice') .then(response => { if (!response.ok) { throw new Error("Network response was not ok") } return response.json(); }) .then(data => { dynamicIdContainer.innerHTML = `Advice # ${data.slip.id}`; dynamicQuoteContainer.innerHTML = `“${data.slip.advice}”`; // Stop the rotation animation when data is received button.style.animation = 'none'; }) .catch(error => { console.log('There was a problem with the fetch operation:', error.message); // Stop the rotation animation upon a fetch error button.style.animation = 'none'; }); }; retriveQuote();I hope you find these suggestions helpful! Let me know if you have any questions or if there's anything else I can assist with.
