created first project #52
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Hi Simone!
Nice first project! I left you some comments on how you could improve the code. On top of that, I think you could review your design/layout choices while having the user experience in mind. For example the images are very big and you have to scroll to see the text, which is very small. Since there's not so much content in each page, it would be great to see all on it at first glance.
Like I said, good job! 🌞
| <div class="dropdown"> | ||
| <a class="drop white" href="./index.html">NACHWUCHS <i class="arrow down"></i></a> | ||
| <div class="dropdown-content"> | ||
| <a class="white" href="./jo.html">JO</a><br> |
There was a problem hiding this comment.
There are some other ways to ensure all your a tags here end up on different lines (wrapping each of them in a div for example).
| Wir bieten Ski und Snowboard als Breiten- und Rennsport sowie ein Konditionstraining an. | ||
| Von 4 - 7 Jahren gehen die Kinder in die Piccolo. Ab 7 Jahren gehören sie der JO an.</p> | ||
| </div> | ||
| <footer> |
There was a problem hiding this comment.
Good use of the header and footer tags!
| <a class="white" href="./termine.html">TERMINE</a> | ||
| <a class="white" href="./kontakt.html">KONTAKT</a> | ||
| </nav> | ||
| <header id="titel"> |
There was a problem hiding this comment.
Since it's standard to use class names more than id for styling purposes, here you could use instead class="titel" (and .titel instead of #titel in css)
Comment on lines
+6
to
+7
| The tab 'Termine' is supposed to be interactive and with some links, but I havn't been able to do that yet. | ||
| Also, I would like to add a registration form in 'Kontakt' and of course the real adresses. |
There was a problem hiding this comment.
Nice to have some goals to improve further your website! 😄
| <section class="kontakt"> | ||
| <div class="name"> | ||
| <p>Urs Kaufmann</p> | ||
| <a href="mailto:[email protected]">MAIL</a> |
| margin: 0; | ||
| } | ||
|
|
||
| h1, h2 { |
There was a problem hiding this comment.
Great that you avoided here to repeat the same code twice for two tags! It's always good to keep our code DRY (don't repeat yourself). You could also give them a class name and it would do the same.
Side note: h1 is usually bigger than h2 (just food for thoughts 😄 ).
|
|
||
| a.black:link, a.black:visited { | ||
| color: black; | ||
| text-decoration: underline; |
There was a problem hiding this comment.
text-decoration: underline; is default behaviour for a tag, so you can remove that line.
Comment on lines
+42
to
+43
| padding-top: 20px; | ||
| padding-right: 40px; |
There was a problem hiding this comment.
There's a shorthand allowing to right it on one line:
padding: 20px 40px 0 0; (the four values are from top to left in clockwise order)
Comment on lines
+50
to
+55
| border: solid whitesmoke; | ||
| border-width: 0 1px 1px 0; | ||
| display: inline-block; | ||
| padding: 3px; | ||
| transform: rotate(45deg); | ||
| -webkit-transform: rotate(45deg); |
Comment on lines
+82
to
+83
| margin-left: 100px; | ||
| margin-right: 100px; |
There was a problem hiding this comment.
Same shorthand as for padding here:
margin: 0 100px 0 100px;
But it can even be shorter when top/bottom and left/right are the same:
margin: 0 100px;
|
Hi Nadia
Thank you very much for your detailed feedback. It helps a lot, I will revise the project for the third part of the course.
Also thanks for the positive comments.
… Am 12.10.2022 um 14:18 schrieb Nadia Lefebvre ***@***.***>:
@nadialefebvre commented on this pull request.
Hi Simone!
Nice first project! I left you some comments on how you could improve the code. On top of that, I think you could review your design/layout choices while having the user experience in mind. For example the images are very big and you have to scroll to see the text, which is very small. Since there's not so much content in each page, it would be great to see all on it at first glance.
Like I said, good job! 🔥
In code/index.html <#52 (comment)>:
> </head>
<body>
- <!--
- Write your code here inside the body to make it appear in the browser.
- Below is an example on how to use an image tag. Copy it and add it in the body, but outside the comment tag if you want to see how it looks like on the website. Then you can change the size and other styling for it in the css file.
-
- <img src="example-recipe-background.png" alt="Banana Bread">
-
- -->
- <h1>Hello there! 👋🏼</h1>
+ <nav>
+ <div class="dropdown">
+ <a class="drop white" href="./index.html">NACHWUCHS <i class="arrow down"></i></a>
+ <div class="dropdown-content">
+ <a class="white" href="./jo.html">JO</a><br>
There are some other ways to ensure all your a tags here end up on different lines (wrapping each of them in a div for example).
In code/index.html <#52 (comment)>:
> + <a class="white" href="./piccolo.html">PICCOLO</a>
+ <a class="white" href="./turnen.html">TURNEN</a>
+ </div>
+ </div>
+ <a class="white" href="./termine.html">TERMINE</a>
+ <a class="white" href="./kontakt.html">KONTAKT</a>
+ </nav>
+ <header id="titel">
+ <h1>JO GOMMISWALD</h1>
+ </header>
+ <div class="about">
+ <p>Die JO Gommiswald vermittelt den Kindern im Alter von 4 - 15 Jahren die Freude am Schneesport.
+ Wir bieten Ski und Snowboard als Breiten- und Rennsport sowie ein Konditionstraining an.
+ Von 4 - 7 Jahren gehen die Kinder in die Piccolo. Ab 7 Jahren gehören sie der JO an.</p>
+ </div>
+ <footer>
Good use of the header and footer tags!
In code/index.html <#52 (comment)>:
> -
- -->
- <h1>Hello there! 👋🏼</h1>
+ <nav>
+ <div class="dropdown">
+ <a class="drop white" href="./index.html">NACHWUCHS <i class="arrow down"></i></a>
+ <div class="dropdown-content">
+ <a class="white" href="./jo.html">JO</a><br>
+ <a class="white" href="./piccolo.html">PICCOLO</a>
+ <a class="white" href="./turnen.html">TURNEN</a>
+ </div>
+ </div>
+ <a class="white" href="./termine.html">TERMINE</a>
+ <a class="white" href="./kontakt.html">KONTAKT</a>
+ </nav>
+ <header id="titel">
Since it's standard to use class names more than id for styling purposes, here you could use instead class="titel" (and .titel instead of #titel in css)
In README.md <#52 (comment)>:
> +The tab 'Termine' is supposed to be interactive and with some links, but I havn't been able to do that yet.
+Also, I would like to add a registration form in 'Kontakt' and of course the real adresses.
Nice to have some goals to improve further your website! 😄
In code/kontakt.html <#52 (comment)>:
> + <div class="dropdown-content">
+ <a class="white" href="./jo.html">JO</a><br>
+ <a class="white" href="./piccolo.html">PICCOLO</a>
+ <a class="white" href="./turnen.html">TURNEN</a>
+ </div>
+ </div>
+ <a class="white" href="./termine.html">TERMINE</a>
+ <a class="white" href="./kontakt.html">KONTAKT</a>
+ </nav>
+ <header>
+ <h2>Kontakt</h2>
+ </header>
+ <section class="kontakt">
+ <div class="name">
+ <p>Urs Kaufmann</p>
+ <a ***@***.***">MAIL</a>
Well done with the mailto: 👍
In code/piccolo.html <#52 (comment)>:
> + </div>
+ <a class="white" href="./termine.html">TERMINE</a>
+ <a class="white" href="./kontakt.html">KONTAKT</a>
+ </nav>
+ <header>
+ <h2>Piccolo</h2>
+ </header>
+ <div class="about">
+ <img src="./images/piccolo.jpeg">
+ <p>Das Piccoloteam gibt den Kindern die Gelegenheit,
+ den spielerischen Umgang mit den Skis zu üben.
+ Dieses Angebot gilt für alle, ob Anfänger oder kleine Rennfahrer,
+ auch für Nichtmitglieder des Skiclub.</p>
+ <p>Das Piccolo-Lied, gesungen von der 2. Klasse von Mirco Büsser in Eschenbach<br><br>
+ <audio controls>
+ <source src="./images/clublied.mp3" type="audio/mpeg">
Nice addition with the mp3! 🔥
In code/jo.html <#52 (comment)>:
> + <p><img src="./images/urs.jpeg" style="width:80px;height:80px;">
+ Ansprechsperson:<br><a class="black" href="./kontakt.html">Urs Kaufmann</a></p>
⬇️ Suggested change
- <p><img src="./images/urs.jpeg" style="width:80px;height:80px;">
- Ansprechsperson:<br><a class="black" href="./kontakt.html">Urs Kaufmann</a></p>
+ <p>
+ <img src="./images/urs.jpeg" style="width:80px;height:80px;">
+ Ansprechsperson:
+ <br>
+ <a class="black" href="./kontakt.html">
+ Urs Kaufmann
+ </a>
+ </p>
You could improve the readability here by changing line for each tag (with proper indentation).
In code/style.css <#52 (comment)>:
> body {
- background-color: lightblue;
+ font-family: 'Roboto', Verdana, Calibri, sans-serif;
+ font-size: 14px;
+ margin: 0;
+}
+
+h1, h2 {
Great that you avoided here to repeat the same code twice for two tags! It's always good to keep our code DRY (don't repeat yourself). You could also give them a class name and it would do the same.
Side note: h1 is usually bigger than h2 (just food for thoughts 😄 ).
In code/style.css <#52 (comment)>:
> - background-color: lightblue;
+ font-family: 'Roboto', Verdana, Calibri, sans-serif;
+ font-size: 14px;
+ margin: 0;
+}
+
+h1, h2 {
+ font-family: 'Segoe UI', Verdana, Calibri, sans-serif;
+ font-size: 50px;
+ text-align: center;
+ margin: 0;
+}
+
+a.black:link, a.black:visited {
+ color: black;
+ text-decoration: underline;
text-decoration: underline; is default behaviour for a tag, so you can remove that line.
In code/style.css <#52 (comment)>:
> + padding-top: 20px;
+ padding-right: 40px;
There's a shorthand allowing to right it on one line:
padding: 20px 40px 0 0; (the four values are from top to left in clockwise order)
In code/style.css <#52 (comment)>:
> + border: solid whitesmoke;
+ border-width: 0 1px 1px 0;
+ display: inline-block;
+ padding: 3px;
+ transform: rotate(45deg);
+ -webkit-transform: rotate(45deg);
Nice job with the arrow 🔽
In code/style.css <#52 (comment)>:
> + margin-left: 100px;
+ margin-right: 100px;
Same shorthand as for padding here:
margin: 0 100px 0 100px;
But it can even be shorter when top/bottom and left/right are the same:
margin: 0 100px;
—
Reply to this email directly, view it on GitHub <#52 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A27M3ML75ICW4WAEB7NLLDLWC2UAZANCNFSM6AAAAAAQZSTGLU>.
You are receiving this because you authored the thread.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.