Skip to content

Conversation

@VKNS
Copy link

@VKNS VKNS commented Jun 28, 2019

28.06.2019
дз по верстке ч.1
03.06.2019
дз по верстке ч.2

}

body {
font-family: 'AvenirNext-Bold';
Copy link

Choose a reason for hiding this comment

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

You should always use some default fallback font or fonts since in some cases your additional fonts could be unavailable.

Copy link
Author

@VKNS VKNS Jul 23, 2019

Choose a reason for hiding this comment

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

i edited homework according comments
i just pushed them, so you can see changed files
Or should i make a new pull request or ...?

Copy link
Author

Choose a reason for hiding this comment

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

changed

Copy link

@svvald svvald Jul 29, 2019

Choose a reason for hiding this comment

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

Pull request automatically reflects all the changes you push so there is no need to create new pull request

width: 800px;
}

.bg2 {
Copy link

Choose a reason for hiding this comment

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

I don't really like this and previous classnames, I'd make them more meaningful.

Copy link
Author

Choose a reason for hiding this comment

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

changed

height: 600px;
width: 800px;

display: -webkit-box;
Copy link

@svvald svvald Jul 22, 2019

Choose a reason for hiding this comment

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

Strange delimiters between rules. I'd group some of them together, e.g. rule and its vendor-specific variants.
Also, it is better to place default rule at first and then list its prefixed options.

Copy link
Author

Choose a reason for hiding this comment

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

changed

<body>
<div class="bg1">
<div class="bg2">
<article class="widget">
Copy link

Choose a reason for hiding this comment

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

You used semantic tag once but why didn't you continue do it through all the file?
Also I don't think that article is an appropriate tag for this widget.

Copy link
Author

@VKNS VKNS Jul 23, 2019

Choose a reason for hiding this comment

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

I thought article is for situations when content can be cut from the page without loosing meaning. i thought it's such а situation
What tag fits here best?

Copy link

Choose a reason for hiding this comment

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

Well, article fits better for some independent text content that could be repeat but is not obliged to be presented on the page like blog post or comment. In your case widget is the essential part of the page. I guess that main tag will be better

margin-top: 26px;
}

.card__1 {
Copy link

Choose a reason for hiding this comment

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

These are meaningless classnames. You could name them like card__blue etc. based on the color.

Copy link
Author

Choose a reason for hiding this comment

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

changed

Copy link

@svvald svvald left a comment

Choose a reason for hiding this comment

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

It looks good now

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