Skip to content

Finish homework 01#12

Open
JacintheChen wants to merge 1 commit intodistinctioncoding:mainfrom
JacintheChen:main
Open

Finish homework 01#12
JacintheChen wants to merge 1 commit intodistinctioncoding:mainfrom
JacintheChen:main

Conversation

@JacintheChen
Copy link

No description provided.

Copy link

@Gary-Distinctioncoding Gary-Distinctioncoding left a comment

Choose a reason for hiding this comment

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

Overall is good, Consider using more meaningful class names, e.g. left-section and right-section instead of left and card. Also, try to control the gaps in flexbox for responsive design in your next homework

}

.left {
text-align: c

Choose a reason for hiding this comment

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

this value is invalid

<div class="page">
<div class="left">
<h1>Welcome to Paint Quote System</h1>
<div class="line"></div>

Choose a reason for hiding this comment

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

Can use ::after to avoid adding an additional element, as ::after is a pseudo-element that belongs to an element, so it can be attached to the h1

Comment on lines +24 to +39
<div class="form-group">
<label for="name">Your name</label>
<input id="name" type="text" placeholder="Enter your name" />
</div>

<div class="form-group">
<label for="phone">Phone number</label>
<input id="phone" type="tel" placeholder="Enter your phone number" />
</div>

<div class="form-group">
<label for="dulux">Dulux account</label>
<input id="dulux" type="text" placeholder="Enter your Dulux account" />
</div>

<button type="button">Sign up</button>

Choose a reason for hiding this comment

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

Can wrap all the inputs and button in a form element for better practice, as browsers havedefault behaviours for form element

Comment on lines +14 to +20
.page {
display: flex;
justify-content: space-between;
align-items: center;
padding: 40px;
min-height: 100vh;
}

Choose a reason for hiding this comment

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

  1. usually min-height is set on the body element, as every page has body element but not every page has .page class
  2. can set a max-width as the current space-between causes the left and card to be too far apart on large screens, or you can use gap or relative unit (%) for better pracrtice

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