Kevin Powell - Community•11mo ago

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

Takyon_is_OnlineOP•11mo ago

This is how the website is supposed to look. I only did HTML and CSS so far.

Takyon_is_OnlineOP•11mo ago

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.

Coder_Carl•11mo 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_OnlineOP•11mo ago

Sorry for the late response I will read it over.

dys 🐙•11mo 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_OnlineOP•11mo 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•11mo ago

Does your svg have a fill/stroke in the file?

Takyon_is_OnlineOP•11mo 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•11mo ago

You have list style none in your css. Remove that. Sorry I must have missed that step

Takyon_is_OnlineOP•11mo 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•11mo 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_OnlineOP•11mo ago

Ok thanks, I will be around then. I'm going to start a new project then.

Coder_Carl•11mo 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_OnlineOP•11mo 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•11mo ago

do you mean another branch?

Takyon_is_OnlineOP•11mo ago

Yeah that's what it was

Coder_Carl•11mo 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_OnlineOP•11mo 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!

Gaming

Programming

Can some review my attempt at this coding challenge?