Feedback for Tribute Page Please

<%= @topic_view.topic.title %>
<%= @topic_view.topic.average_rating %> <%= @topic_view.topic.posts.count { |p| !!p.custom_fields['rating'] } %>

Hi!

I completed my first project. Any feedback is welcome and appreciated!

Thank you!

https://codepen.io/vinhdoan13/full/XWrYRNJ

1 Like

This looks great but I might be a little biased since I love Tool

3 Likes

Thanks for taking time to look!

Your use of HTML5 semantics is awesome. You do better than many people do on that front. Doing this helps with both readability and screen readers for those who have difficulty seeing a website. Just know that by using these special tags like main, article, section, etc is doing something that helps with people whom are at a disadvantage and doesn’t exclude people from your content.

I would say a few things:

  1. The meta tag is in the head of the DOM which is typically at the very top of the page so that meta tag that helps with responsiveness should be above all of the code. Keeping consistent and popular habits will help other coders understand your code better since they will also share similar habits and will expect certain behaviors which will allow them the ability to do quick glances at your code.

  2. I would try to keep my code properly tabbed. It seems inconsistent throughout. No need to space out things too much.

  3. Try not to use links to images that are several characters long. If you must, save it to your desktop, put it on an image site like imgur or photobucket and then link to it. It looks cleaner this way.

  4. The read more at the bottom of the page is not linking to anything atm.

These are just my opinions. Overall, the layout and style is very pleasing visually.

1 Like

I did notice that you left out a quote mark in one of the image tags

<img id="image2" src="i removed the src" style="width:100% alt="Adam Jones working"</img>

it should be:

 <img id="image2" src="i removed the src" style="width:100%" <!--missed the quote mark here--> alt="Adam Jones working"</img>

or just 

 <img id="image2" src="i removed the src" style="width:100%" alt="Adam Jones working">

2 Likes

That is a good catch! I believe he has done this for all the photos which brings me back to my point of having cleaner code. Catching these things is more efficient with less jargon in the code.

1 Like

Thanks for the detailed feedback!! I’m going to take this to heart, it’s my first project ever and I definitely want to establish good practice.

I will make changes as soon as I can.

Also, the “Tool” logo is the link. When I click the logo it takes you to the website. I will look into it ASAP to see if it’s working.

Much appreciated!

1 Like

Thanks for the proofread!!

1 Like

Hi,

I put the images on to imgur and linked them in my code, which cleared up a lot of clutter, but now the images do not show in a mobile browser. Any ideas?

Thanks

The codepen: https://codepen.io/mikeboutros/pen/ExYpaPp

Hi @vinhdoan13,

I decided to go ahead and make a codepen to fork your code and reorganize it a bit. I also dumped it into jsfiddle to get a second opinion.

Upon looking further into the issue with the images, Imgur does not work for the purpose of using it in codepen. They don’t allow it. Try using dropbox or Github and changing the links again. Even though it works fine on the desktop version of codepen, the mobile version is not working.

Regarding codepen, if you want to see the issues/code errors in your HTML or CSS (among the other languages), you can click on the arrow on the right as seen here:

04%20AM
And then clicking “Analyze HTML” or “Analyze CSS” if you are looking at CSS. I ran the HTML analysis and got the following results:

:arrow_right: HTML

  • The <img> tag does not need a closing tag as per w3schools. So I went ahead and removed them.

  • Line 15 and 34 are missing a > to close the <img> tag. This is due to that the closing tag for <img> made it appear like the opening <img> tag was closed but in reality was still open once I removed the closing tag that does not need to be there.

  • The border attribute on <img> has been deprecated. Even though you have it set to 0, it is best practice not to include it. Some more information here. I have removed them all.

  • On line 20, I changed the <div> to <section> for the sake of keeping it semantic.

  • The <hr> tag does not need a closing tag. This applies to <br> as well. Also when styling a tag, make it a habit to use CSS as it de-clutters your HTML and as a developer, you will want people to admire your work like a piece of art. Some more info here.

<hr id="hr3" size=5px color=#4B0082> </hr>
  • When including inline styles on any of your tags such as the code sample directly below, it is highly suggested that you end the style with a semicolon( ; ). In this case, you would put a semicolon after the % in 100%. It would look like this: <style="width:100%;">. Although it is not necessary to put a semicolon if you have a single style like you do in this case, putting it will create a good habit. It is required that you put a semicolon if you have multiple styles though like color and/or font-family. You can read more here.

<img id="logo" src="image.jpg" width="259" height="55" style="width:100%" alt="Tool Logo">

  • The <figcaption> for the <figure> of id img-div-2 was not in <figure> so I went ahead and put inside of it.

  • The final 2 issues are related to the <hr> tag.

49%20AM
The simplest option here is to make a class or id (which you have already done) and put it into the CSS file so that it coincides with the HTML standards of today. I’ve added your id of hr3 into the CSS file and put the appropriate CSS style inside of it.

  • As a side note, I noticed you have a lot of id’s and classes that are not being used or in the CSS file. I know that for the purpose of this project, they needed to be included in order to pass, but personally I do my best not to overkill my DOM if I can help it. Only create what you plan to use otherwise it should not exist. This is an FYI for personal projects you might work on in the future.

:arrow_right: CSS

  • The CSS file needs to be revised. For example, the border-radius property should be 1 line; not 4 like it is right now. Example of current code:
  border-bottom-left-radius: 50px;
  border-bottom-right-radius: 25px;
  border-top-left-radius: 50px;
  border-top-right-radius: 200px;

HTML works in clockwise motion. So your browser is going to see the border-radius as the following:

00%20AM
In your case, instead of 4 lines of code, we will condense it to 1 like the following:

border-radius: 50px 200px 25px 50px;

border-radius: top left, top right, bottom right, bottom left

I made the change to #image2 in the CSS file for your reference.

  • Be careful of duplicates. There are 2 color styles under the h2 tag both pointing to the color black. I removed one of them.

I’ve ran through all of the issues that were outlined in the orange image from codepen and corrected it all. There is still a lot of minor formatting that can be done but I chose to leave it because at this point of the analysis, everything that was major was corrected. It would be classified as a preference thing due to that it isn’t hindering anyone.

With that being said, good job on the site overall. It has grown on me. :smiley:

-mb

P.S. I wanted to say that I learned a lot while going through your tribute page. There was some stuff I never did/thought of doing like styling <hr>, wrapping my images with <figure> and including a <figcaption> to describe the images. Also in your CSS, I often forget about image manipulation like border-radius. I tend to stick to vanilla and just modify the colors. I’ll think about doing that in the future on my own material.

P.P.S. I refer to line numbers that I modified but failed to tell you that those numbers are inconsistent with my codepen since I ended up changing the flow of the DOM. I would use your codepen as those were the lines I adjusted.

1 Like

You, my friend, are absolutely amazing! Thank you so much for putting the time to help me learn in a constructie and positive way! This helps so much!

1 Like