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.
18 Replies
This is how the website is supposed to look. I only did HTML and CSS so far.
https://github.com/DaveSamuels1998/Newsletter-sign-up-form-challenge
This so you can see the HTML anc CSS
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.
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
The checkmarks on the ul>li should probably use the image as set in the css rather than adding the image 3 times.
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 ...
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
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.
Sorry for the late response I will read it over.
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.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 alsoDoes your svg have a fill/stroke in the file?
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?You have list style none in your css. Remove that. Sorry I must have missed that step
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
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
Ok thanks, I will be around then. I'm going to start a new project then.
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 3Ok 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?
do you mean another branch?
Yeah that's what it was
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>
That's really cool! Thanks for telling me that, I will have to do this in the future then once I land a job!