Can some review my attempt at this coding challenge?

I deployed it on GitHub pages This is the URL. https://davesamuels1998.github.io/Newsletter-sign-up-form-challenge/ I think I was told that I used a lot of width and height values in many places and that could be bad. Any suggestions towards that. I can't think of anything.
No description
18 Replies
Takyon_is_Online
Takyon_is_Online10mo ago
This is how the website is supposed to look. I only did HTML and CSS so far.
Takyon_is_Online
Takyon_is_Online10mo ago
GitHub
GitHub - DaveSamuels1998/Newsletter-sign-up-form-challenge: A newsl...
A newsletter sign-up webpage. Contribute to DaveSamuels1998/Newsletter-sign-up-form-challenge development by creating an account on GitHub.
Coder_Carl
Coder_Carl10mo ago
some initial feedback I would make life easier with images immediately set a width on the image, this allows your flex container to grow to the required width without having to give it a width (which as their feedback suggested is bad practice) I am also updating the height as we dont want it to be too big for the viewport on smaller devices
.container{
display: flex;
/* width: 1000px; */
height: min(100% - 2rem, 40rem);
background-color: var(--light-color);
border-radius: 30px;
}
image {
display: block;
width: 100%;
}
.container{
display: flex;
/* width: 1000px; */
height: min(100% - 2rem, 40rem);
background-color: var(--light-color);
border-radius: 30px;
}
image {
display: block;
width: 100%;
}
The checkmarks on the ul>li should probably use the image as set in the css rather than adding the image 3 times.
ul {
padding-inline-start: 1.5em; /* this keeps the checkmarks in line with the text above it. */
}
ul li {
/* display: flex; */
/* align-items: center; */
/* list-style: none; */
/* margin-bottom: 10px; */
list-style-image: url(newsletter-sign-up-with-success-message-main/assets/images/icon-list.svg)
}
ul {
padding-inline-start: 1.5em; /* this keeps the checkmarks in line with the text above it. */
}
ul li {
/* display: flex; */
/* align-items: center; */
/* list-style: none; */
/* margin-bottom: 10px; */
list-style-image: url(newsletter-sign-up-with-success-message-main/assets/images/icon-list.svg)
}
There is also no need for a div there. we then look and see that you have margins and padding on things all through out in order to fix the position of things but the flex should be positioning things instead so that a) it can reallocate space at different screen sizes and b) we need to write less css in media queries (if any) so ...
.home-image {
flex: 1 1 40%;
/* margin-left: 50px; */
/* height: 580px; */
/* padding-top: 20px; */
}
.home-image {
flex: 1 1 40%;
/* margin-left: 50px; */
/* height: 580px; */
/* padding-top: 20px; */
}
we want to keep spacing consistent on content on the LHS so lets just set a margin on each of the children of header-content
.header-content > *:where(:not(:first-child)) {
margin-top: 1em;
}
.header-content > *:where(:not(:first-child)) {
margin-top: 1em;
}
There are a few of other niggly bits here and there but it honestly depends on how much time and effort you want to put into these sorts of things. As I said you have a lot of margins and padding that probably dont need to be there. I think a lot of what you wrote here you will find yourself fighting against when you set up media queries. honest feedback ⬆️ , all in all your approach ended with the element appearing as you wanted it to so 🤷 but I have given you the above feedback because I think you are intending on using these exercises to get better.
Takyon_is_Online
Takyon_is_Online10mo ago
Sorry for the late response I will read it over.
dys 🐙
dys 🐙10mo ago
You should link your Pages deployment in the URL section of your GitHub repo so people know it's there when looking at your code. Also, at a glance, your spacing isn't consistent. It really degrades the readability of your code. Even if you were just to autoformat in VS Code. You defined a set of colors in :root {} in your CSS. You should ideally define a dark counterpart in a @media (prefers-color-scheme: dark) {} section. The submit button goes inside the <form> tags.
Takyon_is_Online
Takyon_is_Online10mo ago
Ok I was apply some of the changes and ul { list-style-image: url('newsletter-sign-up-with-success-message-main/assets/images/icon-success.svg'); } is not displaying any styling. I checked on Stack Overflow and saw that this should work. I copied the relative path for the .svg file and got that same exact path. Any reason why it's not displaying anything? I remove the img takes from the HTML also
clevermissfox
clevermissfox10mo ago
Does your svg have a fill/stroke in the file?
Takyon_is_Online
Takyon_is_Online10mo ago
So I moved all my files because my html and css was in my root folder. So my path for my images are start with assest. For the image on the right it displays when I put the path as src="assets/images/illustration-sign-up-desktop.svg" For list image I want to use it is located in the same folder as "llustration-sign-up-desktop.svg" It doesn't display when I put list-style-image: url("assets/images/icon-success.svg"); Why doesn't the path for src work for the url() property in CSS?
Coder_Carl
Coder_Carl10mo ago
You have list style none in your css. Remove that. Sorry I must have missed that step
Takyon_is_Online
Takyon_is_Online10mo ago
I did, still didn't work. I missed that too and thought it would have worked but it didn't. That's why I am so confused
Coder_Carl
Coder_Carl10mo ago
I'm not on my comp right now so can't see your code to check what it is. Will be an hour or so
Takyon_is_Online
Takyon_is_Online10mo ago
Ok thanks, I will be around then. I'm going to start a new project then.
Coder_Carl
Coder_Carl10mo ago
I see the problem. one of my comments was making it appear that I was suggesting to add the list style property to the ul. I have now closed off that comment so that it doesnt remove the ul li selector that is not the case, you will need to add it to the li the checkmarks will appear quite large, but realistically you shouldnt be serving images that are too large. you can 1 just serve the correct size image and then be done with it 2 use a before element and then style it as you had done previously (this can give you more control) 3 use list-style-type: "✔"; and add a :before element that looks like a circle and position it behind the checkmark one of those options is super easy imo. only if the layout isnt working would I reach into option2 or 3
Takyon_is_Online
Takyon_is_Online10mo ago
Ok I will implement that now. A side note, I think I added another repository to my project by accident. Do I have to delete the entire repository to fix this?
Coder_Carl
Coder_Carl10mo ago
do you mean another branch?
Takyon_is_Online
Takyon_is_Online10mo ago
Yeah that's what it was
Coder_Carl
Coder_Carl10mo ago
a github repo can have thousands of branches. its good practice to prune them sometimes, but 🤷 typically on your local machine you would create a branch, git checkout -b <new-branch-name> then complete your work git checkout main git merge <branch-name> git add <specific files, or just a .> git commit -m "UPDATE whatever you did" git push then delete the branch git branch -d <branch-name>
Takyon_is_Online
Takyon_is_Online10mo ago
That's really cool! Thanks for telling me that, I will have to do this in the future then once I land a job!
Want results from more Discord servers?
Add your server