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

Develop #4924

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Develop #4924

wants to merge 3 commits into from

Conversation

om1gse
Copy link

@om1gse om1gse commented May 2, 2024

  • DEMO LINK

  • TEST REPORT LINK

  • Header height is set in 1 place (for the links)

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

Copy link

@dinaminahi dinaminahi left a comment

Choose a reason for hiding this comment

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

Well done 👍
Let's make it a little bit better

Choose a reason for hiding this comment

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

Please, remove this and let's use existing logo image in src/images folder

src/index.html Outdated
Comment on lines 35 to 39
<img
src="./images/logo.png"
alt="Moyo logo"
class="logo"
/>

Choose a reason for hiding this comment

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

Not critical, but better to format it
image

src/style.css Outdated
html {
font-family: Roboto, sans-serif;
font-weight: 500;
font-style: normal;

Choose a reason for hiding this comment

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

Suggested change
font-style: normal;

It is default style

src/style.css Outdated
Comment on lines 5 to 7
* {
box-sizing: border-box;
}

Choose a reason for hiding this comment

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

image

@om1gse om1gse closed this May 3, 2024
@om1gse om1gse reopened this May 3, 2024
@om1gse om1gse requested a review from dinaminahi May 3, 2024 16:08
Copy link

@dinaminahi dinaminahi left a comment

Choose a reason for hiding this comment

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

Well done 👍
just pay attention to the existing files, you don't have to import your own image here

Choose a reason for hiding this comment

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

Let's remove this and use the image from src/images folder

Comment on lines +42 to +44
<a
href="#"
>

Choose a reason for hiding this comment

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

If we have 1 or two attributes we place it in one line

Suggested change
<a
href="#"
>
<a href="#">

Choose a reason for hiding this comment

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

applies to all code

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