Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

html-css-popup #10

Merged
merged 3 commits into from
Jul 27, 2022
Merged

html-css-popup #10

merged 3 commits into from
Jul 27, 2022

Conversation

OlStani
Copy link
Contributor

@OlStani OlStani commented Jul 26, 2022

HTML CSS Popup

Demo |
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

Copy link

@yaripey yaripey left a comment

Choose a reason for hiding this comment

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

Nice custom design. Let's see how you can improve your work.

@@ -0,0 +1,203 @@
@charset "UTF-8";
Copy link

Choose a reason for hiding this comment

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

Don't add autogenerated files into the homeworks folder. Only files you yourself typed in.

font-weight: 400;
}

/*--------------------*/
Copy link

Choose a reason for hiding this comment

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

If you want to have dividers of some sort, it is also nice to name them. Like

/*----------Header----------*/

or something like that.

font-weight: bold;
}
}

Copy link

Choose a reason for hiding this comment

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

Empty line.

max-height: 70vh;
overflow: hidden;
overflow-y: auto;

Copy link

Choose a reason for hiding this comment

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

Empty line.

margin: 0 auto;
position: relative;


Copy link

Choose a reason for hiding this comment

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

Two empty lines. It is fine to leave an empty line between two blocks, but be consistant, you have only one line between other blocks.

<div class="header__wrapper_logo">FOOD DELIVERY</div>
<div class="header__wrapper_checkbox headerInput">
<label for="drop">Choose Delivery</label>

Copy link

Choose a reason for hiding this comment

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

Empty line.

<div class="main__wrapper_item mainList">
<ul class="mainList__list">
<li class="mainList__list_item listItem"><a class="imageAnchor" href="#"><img
src="./img/mcdonalds.svg" alt="mcdonalds"></a></li>
Copy link

Choose a reason for hiding this comment

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

alt attribute for navigation elements, like icons, should be left empty.

@Roophee
Copy link
Contributor

Roophee commented Jul 26, 2022

Good work!
Not bad design but you can make it better with responsive styles for mobile devices.
This up to you, but it will be look more perfect and complete with the responsive styles.

@OlStani OlStani requested a review from yaripey July 27, 2022 13:37
@yaripey
Copy link

yaripey commented Jul 27, 2022

Nice work!

@yaripey yaripey merged commit ef7d46a into kottans:main Jul 27, 2022
@OlStani
Copy link
Contributor Author

OlStani commented Jul 27, 2022

thanks!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants