Skip to content
OverviewSolutions
13
Comments
11
Manuel Gil
@ManuGil22

All comments

  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey there! Pretty cool solution! Have one thing to say as feedback:

    • Always use heading tags in order! You should use <h1> first and if the font its too big u can always style it with css!
    <h1 class="card__name">Victor Crest</h1>

    Everything else seems pretty good for me! Well done :D

    0
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @hrishikdm pretty cool solution! Have some things to say as feedback:

    • Wrap all body content in a <main> tag for accessibility purposes.
    • Always use heading tags in order. Dont use <h2> or <h5> as a first tag, you can use a <h1> instead and change the text-style with css!
    <h1>How did we do?</h1>

    Also for the text area were u have h5 and h2!

    Marked as helpful
    1
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @Danny-Rodriguez pretty cool solution! Have only one thing to say as feedback:

    • Always use heading tags in order. Dont use <h4> as a first tag, you can use a <h1> instead and change the text-style with css!
    Marked as helpful
    2
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @AbhishekBh72

    To position a element in the exact center of a page you can use this attributes on the body element:

    • height: 100vh; //To make the height of the body 100 viewheight units
    • display: flex; //To use flexbox
    • justify-content: center; //To center elements horizontally
    • align-items: center; //To make all items center in the page vertically

    I hope I answered your question! Keep pushing and happy coding!

    Marked as helpful
    1
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @victorsonet !

    Some feedback:

    • Your media queries are not working properly. Maybe its better if you wrap the qr-section and content-section in one container and work with that container size. I would probably do a max-width: 350px for that container and u wont need to add queries. However u will have to change some of ur css in order to do that.

    This is a link to my solution: Frontend Mentor solution

    If you want u can take a look at it and ask me anything u need.

    Keep pushing and happy coding!

    Marked as helpful
    1
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @DevBaddy ! Good work there! Your repo link its not working tho, cant check the code to give any feedback. However it seems all good. Try to add a <main> tag around the code for accessibilities purposes!

    Well done and happy coding!

    0
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @gcapistrano your solution is pretty good! Just some details u might wanna change:

    • Add a <main> tag around the main code
    • Try to match colors of the desing. I think the h1 should be like a dark blue or smth like that.

    Really well done!

    Keep pushing and happy coding!

    Marked as helpful
    1
  • Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey @Escobar23 good job with the challenge!

    Just some details to check:

    • Try to decrease the margin-top for the .card, 200px seems like too much and if u check it on the phone u will see its a lot of margin. Maybe u can try with margin-top: 8rem
    • Add a <main> tag around the div with card class to solve the accessibility issue.

    Everything is just perfect, dont worry about the sizes of the card, there are just details and u will improve it for sure!

    Keep pushing and happy coding!

    Marked as helpful
    0
  • John Raily Doe•50
    @hopzinga22Submitted over 2 years ago

    This project was quite difficult but I managed to complete it.

    Any criqs will be appreciated.

    Nft-card challenge

    1
    Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey there @hopzinga22

    Have some feedback for you.

    • Dont use <h4> tags for the price and the time left. Heading levels should increase by one. U should use <h2> or maybe smth else like a <p> or <span>. Also try to fix this on the creator section.
    • When hover on the nft-name and the creators-name change the cursor to pointer and the color of the text. Can do it like this for the nft-name:
    h1:hover {
    cursor: pointer;
    color: (#cyan color);
}

    And for the creators-name u can add a span around it:

    • html
    <h5><span>Creation of </span><span class="creator-name">Jules Wyvern</span></h5>
    • css
    .creator-name {
    cursor: pointer;
    color: (#cyan color);
}
    • You can also add this lines to class time-amount in the css file to make the time left move to the right:
    .time-amount {
    display: flex;   // this line was already in ur code
    justify-content: space-between;
    width: 90%;
}

    There are just details, you have done it fantastic!

    Keep pushing and happy coding!

    Marked as helpful
    0
  • peta•110
    @petaa33Submitted over 2 years ago

    I have problems with inactive ratings not changing their background color to dark-blue once they are not active, instead they stay light-grey. Any feedback and help is welcome.

    HTML, CSS, Javascript

    1
    Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey there @peta !

    What i did was to create a class called --checked. With css added there all the styles for the rating buttons when clicked. Then with js, in the eventListener for rating, removed the class --checked for all rating buttons, and added it to the one clicked.

    • CSS
    .rate-btn--checked {
    background-color: hsl(217, 12%, 63%);
    color: hsl(0, 0%, 100%);
}
    • JS
    rateBtns.forEach((btn) => {
	btn.addEventListener('click', (e) => {
		// removes checked class from all rating buttons
		rateBtns.forEach((button) => {
			button.classList.remove('rate-btn--checked');
			button.ariaChecked = 'false';
		});
		// adds checked class to clicked button
		e.target.classList.add('rate-btn--checked');
		e.target.ariaChecked = 'true';
	});
});
    • Also if u can try to add and alternative text to the <img> tags. Its a description of the images for the screenreaders!

    Well done and keep coding!

    0
  • Kohsey•40
    @KohseyPowerSubmitted over 2 years ago

    Feedback welcome ! ^^

    This is my first submition. I'm trying to learn and practice HTML and CSS. There are probably bad practises, notice me !

    HTML and CSS using flexbox

    3
    Manuel Gil•340
    @ManuGil22Posted over 2 years ago

    Hey Kohsey!

    Im pretty new to html and css too but have some feedback:

    • Im not sure which aproach is correct but i would rather use an <h1> tag instead of a <p> for the title.
    • Not need to have <div> outside the paragraphs. As I told you before im pretty new too but think its just no needed. Divs are mostly used to customize and style some sections, for e.g. if you want to change the background-color of the title. In this case you are not doing anything with them so, there just extra lines.
    • One of the accessibility issues you have is that every <img> tag should have an alternate text. Its used by screenreaders as a description of the image. I would do smth like alt="qr code".
    • Also u have an accessibility issue that its not important but u might want to fix it. It says you should have a main landmark in ur code. That can be easily fixed by adding a <main> tag around the div with id="block". Just like this:
    <main>
        <div id="block">
            ....
        </div>
</main>

    There are all details, nothing big. You made an amazing work there!

    Keep pushing and happy coding!

    Marked as helpful
    3