Skip to content

Conversation

@basmalainine
Copy link

@basmalainine basmalainine commented Dec 7, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

</p>
<a href="">Read more</a>
</article>
</main>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to use the <main> element and wrap the whole section till before the <footer>, meaning include inside the <main> all <section> elements

<main>
<article>
<img
src="https://assets.pinterest.com/ext/embed.html?id=492649946889933" height="636" width="345" frameborder="0" scrolling="no" alt=""/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good practice to always include an alt tag including a description usually of the image like alt="Portrait of a lady..." in the <img> elements, this is for accessibility purposes and it is strongly recommended

<div class="content">
<h2>Isaac de Moucheron (Amsterdam 1667-1744 Amsterdam)</h2>
<p>Isaac de Moucheron (Amsterdam, 1667–1744) was a Dutch painter and draughtsman best known for his elegant **Italianate landscape paintings**. Influenced by French classicism and artists like Gaspard Dughet, his works often depict idealized ruins, harbors, and pastoral scenes bathed in warm light. De Moucheron’s landscapes emphasize balance, atmosphere, and architectural detail, reflecting the Dutch Golden Age fascination with travel, nature, and classical antiquity.</p>
<button>Read more</button>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment but sometimes you use a link element <a> and sometimes a <button> for the Read More, usually if you want to navigate to a new page it's better to use a link

</article>
</main>
<section class="article featured">
<div class="https://assets.pinterest.com/ext/embed.html?id=3237030974549846" height="517" width="345" frameborder="0" scrolling="no"></div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you wanted to use an element here instead of a <div>? like above, cause this is not going to smake the image to appear

</div>
</section>
<section class="article-grid">
<div class="article">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for future reference and maybe a good practice for the class names https://getbem.com/introduction/

@azervou
Copy link

azervou commented Jan 16, 2026

One more generic thing, file extension looks missing. Using .html / .css helps editors, and syntax highlighting work correctly.

Overall nice work on this PR, the layout is clear, the content reads well, and the structure is going in the correct direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants