html markup without styles #3
Conversation
| @@ -0,0 +1,2 @@ | |||
| .DS_Store | |||
| Vladimir Kulikov/node_modules | |||
There was a problem hiding this comment.
You have script to create style.css file. That's why it also should be added in gitignore
There was a problem hiding this comment.
Sure, I just thought maybe you will not download this repo and compiled css files will be useful. Fixed)
Vladimir Kulikov/index.html
Outdated
| <article class="weather-widget"> | ||
| <section class="weather-widget__top"> | ||
| <div class="weather-widget__image"> | ||
| <img src="img/cloudy.png" alt=""> |
There was a problem hiding this comment.
alt can't be empty.
if image isn't loaded, user will see this text
Vladimir Kulikov/sass/main.scss
Outdated
| @include font('SourceSansPro-SemiBold'); | ||
| @include font('OpenSans-Regular'); | ||
|
|
||
| html { |
There was a problem hiding this comment.
Try to avoid adding css rule on html tag
Vladimir Kulikov/sass/main.scss
Outdated
| height: 100%; | ||
| } | ||
|
|
||
| body { |
There was a problem hiding this comment.
Here https://www.w3.org/Style/Examples/007/center.ru.html you can read how you can locate you widget in the center
Vladimir Kulikov/sass/main.scss
Outdated
| box-shadow: 0 21px 19px rgba(0, 0, 0, 0.15); | ||
| } | ||
|
|
||
| .weather-widget__top { |
There was a problem hiding this comment.
If you use sccs, you can write like this:
.weather-widget {
&__top {
...
}
}
Vladimir Kulikov/sass/main.scss
Outdated
| .weather-widget__dot { | ||
| width: 6px; | ||
| height: 6px; | ||
| border: 2px solid #565a5b; |
Vladimir Kulikov/sass/main.scss
Outdated
|
|
||
| .weather-widget__dot_active { | ||
| @extend .weather-widget__dot; | ||
| background: #eee; |
Vladimir Kulikov/index.html
Outdated
| <section class="weather-widget__top"> | ||
| <div class="weather-widget__image"> | ||
| <img src="img/cloudy.png" alt=""> | ||
| <button></button> |
There was a problem hiding this comment.
This button doesn't have any text. If some problems with image downloading, user doesn't see anything. It's better to use here and fill alt.
And anyway you should add class for button to write css rule
There was a problem hiding this comment.
Completely agree, alt is very useful especially on buttons without text.
Vladimir Kulikov/index.html
Outdated
| <span class="weather-widget__clear">83%</span> | ||
| </div> | ||
| <div class="weather-widget__pagination"> | ||
| <span class="weather-widget__dot weather-widget__dot_active"></span> |
There was a problem hiding this comment.
You use BEM. Then active is modifier. This class should be called weather-widget__dot--active
Vladimir Kulikov/sass/main.scss
Outdated
| .weather-widget__top-info { | ||
| @include flex(space-between, center); | ||
| padding-bottom: 5%; | ||
| font-family: 'ProximaNova-Light'; |
There was a problem hiding this comment.
It would be cool to add fallback font
https://css-tricks.com/css-basics-fallback-font-stacks-robust-web-typography/
| <section class="weather-widget__top"> | ||
| <div class="weather-widget__image"> | ||
| <img class="weather-widget__image-img" src="img/cloudy.png" alt="cloudy weather image"> | ||
| <button class="weather-widget__image-btn" alt="update"></button> |
There was a problem hiding this comment.
Sorry, I misprinted. I meant
<button> <img alt='...'> </button>
Tag button doesn't have attribute alt
| <article class="weather-widget"> | ||
| <section class="weather-widget__top"> | ||
| <div class="weather-widget__image"> | ||
| <img class="weather-widget__image-img" src="img/cloudy.png" alt="cloudy weather image"> |
There was a problem hiding this comment.
Class names for image and button are strange
2 participants
No description provided.