Skip to content

Conversation

@notnotzero
Copy link

No description provided.

@notnotzero
Copy link
Author

Не могу сдвинуть надпись Sicklerville, New Jersey ниже:(
буду рад любым подсказкам

Copy link

@Adoree Adoree left a comment

Choose a reason for hiding this comment

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

Nicely done!
There is one remark though, seems like widget is not centered horizontally and slightly moved to the right

Сделать два спана вместо <br /> и дать паддинг спану с городом должны помочь решить проблему, если еще актуально

<img src="refresh.png" alt="" />
</div>
<div class="wheatherImage">
<img src="cloud_icon.png" />
Copy link

Choose a reason for hiding this comment

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

Images should have alt attribute, empty or with the short description

>
</div>
<div class="dayInfo">
<span class="topDayText">MAY</span>
Copy link

Choose a reason for hiding this comment

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

It's not necessary to write text in uppercase, consider using styles for that instead

</head>
<body>
<div class="grid">
<div class="wheatherWidget">
Copy link

@Adoree Adoree Jul 29, 2019

Choose a reason for hiding this comment

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

It's convenient to write class names in kebab case (hyphen case) or BEM

<div class="infoBlock">
<img src="tempIcon.png" class="tempIcon" />
<div class="infoText">
<span class="infoTextTop">Cloudy Skies</span><br /><span
Copy link

Choose a reason for hiding this comment

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

Since it's not address, and the first part of the text is related to the weather and another to the location, I'd suggest to avoid using br in such cases, the common use case for br is full address with street / apartment / city etc, also it will become easier to style

</div>
</div>

<div class="pagination">
Copy link

Choose a reason for hiding this comment

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

You should use more semantic tags
In this case you have list of dots, which can be converted to <ul> with multiple <li> inside

letter-spacing: 1px;
color: white;

margin-left: auto;
Copy link

Choose a reason for hiding this comment

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

Try to keep in mind order of properties, group them by type
1 - layout (position, display)
2 - anything related to box model (width/height/margin/padding)
3 - styling for element (color, background, box-shadow)
4 - anything related to font
5 - other stuff, it also includes overflow, z-index

</div>
<div class="humidity">
<img src="waterdrop.png" alt="" />
<span><b>33</b>%</span>
Copy link

Choose a reason for hiding this comment

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

It's convenient to avoid tags like <b> and change appearance of the text through css

<img src="cloud_icon.png" />
</div>
<div class="infoBlock">
<img src="tempIcon.png" class="tempIcon" />
Copy link

Choose a reason for hiding this comment

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

Please, use plain text instead of image

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