Skip to content

Conversation

@mikaelasturk
Copy link

<a href="puppies.html">Valpar</a>
<a href="contact.html">Kontakt</a>
</div>
</header>

Choose a reason for hiding this comment

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

Great organizing of a navigation, very clean!


<h2>På vår hemsida kan läsa om uppkommande kullar, vår uppfödning och andra nyheter!</h2>

<section class="card-grid">

Choose a reason for hiding this comment

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

Looks like a very clean use of semantics, good work!


<label>

<span>För- och efternamn</span>

Choose a reason for hiding this comment

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

I am wondering why you chose to add span inside of label? Maybe I need to consider it too:)


</form>

<footer>

Choose a reason for hiding this comment

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

Overall, very clean and easy to follow structure, good job, keep it up!

style.css Outdated
Desktop >1024*/

body {
height: 100%;

Choose a reason for hiding this comment

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

I'm not sure you need to give the height to the body, it is usually set on full height of the content by default, so I'd suggest to just save some time and code lines:)

box-sizing: border-box;
}

body {

Choose a reason for hiding this comment

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

I think it's great that you have thought about a lot of style details for everything on the page! Also, it is not necessary to repeat body twice, it should be fine to write everything under the same selector


body {
font-family: sans-serif;
line-height: 1.5;

Choose a reason for hiding this comment

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

Line height 1.5 for the paragraph is great and looks very good on your page, and for headers the common practice is to set a bit less, but you can overwrite it in the headers styling, of course

box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1);
}


Choose a reason for hiding this comment

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

Just really clean and well-organized file, keep it up!:)

style.css Outdated

.card-grid {
display: grid;
grid-template-columns: repeat(auto-fit, minmax(250px, 1fr));

Choose a reason for hiding this comment

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

Great card greed!

<input type="text" placeholder="...">
</label>

<label>

Choose a reason for hiding this comment

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

Empty label is a bit confusing since it's unclear what this label is for, would be good to specify it maybe. All other inputs seems very clear, and great that they have placeholders making it easier for users!

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Nice usage of CSS Grid! Would be nice to see you play around with Flexbox some more going forward 🥳

I found a little bug: Make sure your hamburger menu is closable.

Change request

  • Make sure hamburger menu is closable on small phones

Comment on lines +33 to +38
<footer>
<a target="_blank" href="https://instagram.com">Instagram</a>
<a target="_blank" href="https://facebook.com">Facebook</a>
<a target="_blank" href="https://skk.se">SKK</a>
<a target="_blank" href="https://lagottoklubben.se">SLRK</a>
</footer>

Choose a reason for hiding this comment

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

Looks great! 🌟 The footer is clean and minimalistic — I really like the consistent structure of the links and how you’ve used target="_blank" to open them in new tabs. It’s simple, readable, and functional.

index.html Outdated
Comment on lines 27 to 41
<nav class="nav nav-mobile">
<a href="index.html">HEM</a>
<a href="kennel.html">KENNEL</a>
<a href="puppies.html">VALPAR</a>
<a href="contact.html">KONTAKT</a>
</nav>

</details>

<nav class="nav nav-desktop">
<a href="index.html">HEM</a>
<a href="kennel.html">KENNEL</a>
<a href="puppies.html">VALPAR</a>
<a href="contact.html">KONTAKT</a>
</nav>

Choose a reason for hiding this comment

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

The navigation structure is clear and well-organized — each link is straightforward and easy to understand.

style.css Outdated
Comment on lines 214 to 234
.nav {
box-sizing: border-box;
display: flex;
flex-direction: row;
position: sticky;
inset: 0;
background: #6B8E62;
justify-content: center;
padding: 10px;
}

.nav a {
text-decoration: none;
padding: 6px 0;
background-color: #efe0c3;
color: #2b3a28;
margin: 0px;
border-radius: 4px;
font-weight: bold;
padding: 10px 40px;
}

Choose a reason for hiding this comment

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

There are two padding declarations in .nav a — you can safely remove the first one (padding: 6px 0;) since it’s overridden by padding: 10px 40px; 😌

@mikaelasturk
Copy link
Author

Updated with bug fixed in ham-menu and added shadow on cards. @HIPPIEKICK

Copy link

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

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

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.

5 participants