photomed21 / prj-rev-bwfs-dasmoto

0 stars 0 forks source link

Containers Around Page Components #1

Open jordanwillis opened 6 years ago

jordanwillis commented 6 years ago

Nice job having the foresight to include a container for each page component (two or more elements that work together for a single purpose or are contextually associated). However, instead of only including the heading in the container, you should include all elements that make up the "item listing" component.

https://github.com/photomed21/prj-rev-bwfs-dasmoto/blob/943e0c0750aa4edfd8cf1c7b8ecaa21c15411978/Dasmoto/index.html#L11-L13

Here is an example of what I mean.

<div class="item">
  <h2 id="brush">Brushes</h2>
  <img src="https://s3.amazonaws.com/codecademy-content/courses/freelance-1/unit-2/hacksaw.jpeg"/>
  <h3>Hacksaw Brushes</h3>
  <p>Made of the highest quality oak, Hacksaw brushes are known for their weight and ability to hold paint in large amounts. Available in different sizes. <span class="price">Starting at $3.00 / brush.</span></p>
</div>

Since all of these elements are used together to display a single art store product listing, then we should include them together as a single component. The main reason for this is because 9 times out of 10 you will want to apply styling directly the the component (or specific elements within the component), so the container provides you with that capability. It also ensures that you HTML is very semantic since the structure demonstrates the purpose/behavior of the group of elements.

Also, notice that we gave the component a more generic name (e.g. class="item") since there are many other "item listings" on the page. Even though the specific content of component is different, all 3 are still the same component since they represent the same "thing" (e.g. a store listing).

Lastly, keep in mind that any element can have and id or class, so there is no need to have to wrap a container just around your heading element like this.

https://github.com/photomed21/prj-rev-bwfs-dasmoto/blob/943e0c0750aa4edfd8cf1c7b8ecaa21c15411978/Dasmoto/index.html#L18-L20

Instead, you can just add a class like this:

<h2 class="frames">Frames</h2>
photomed21 commented 6 years ago

Thank you for the feedback.