Skip to content
OverviewSolutions
14
Comments
8
Angel M
@csmurillo

All comments

  • Angel M•370
    @csmurilloPosted almost 2 years ago

    Hello Alana, First of all good job in completing the Ping Coming Soon challenge. I went through your code and I would suggest the following:

    1. Add this css to your style sheet .icon-container:hover i{color:white;} this will fix the problem that the icon container background turns blue which covers the icon with blue.

    2. For your email verification I would say your implementation is valid. Some people may remove the error when the form is not on focus. So I would do something like this:(feel free to use)

    let subscribeForm = document.getElementById('emailForm')
let errorMsg = document.getElementById('errorMsg')
let emailInput = document.getElementById('email-input')
//things I added (remove comment)
emailInput.addEventListener('blur',()=>{
  emailInput.classList.remove('inputError')
  errorMsg.style.display='none';
});
emailInput.addEventListener('keydown',()=>{
  emailInput.classList.remove('inputError')
  errorMsg.style.display='none';
});
subscribeForm.children[1].addEventListener('blur', (e)=>{
  emailInput.classList.remove('inputError')
  errorMsg.style.display='none';
});
//end of things i added (remove this)

subscribeForm.addEventListener('submit', (e)=>{
    e.preventDefault()
    if(emailInput.value=="" || !(validator.isEmail(emailInput.value))){
        emailInput.classList.add('inputError')
        errorMsg.style.display='block'
    }
    else{
        emailInput.classList.remove('inputError')
        errorMsg.style.display='none'
    }
})

    Also add this to your stylesheet

    .inputError:focus{
    outline: none !important;
    border: 1px solid red !important;
}

    Some things that may be useful: Pixel vs REM:Article

    More on this: Whats your go to way to go about email validation??? Some people may use stackoverflow to find the result to not use the npm packages available. But most people just use email validation npm packages available in their pure js, react, angular, vue app.(In my experience I have seen this)

    Any more questions just reply to this message.

    0
  • Angel M•370
    @csmurilloPosted almost 2 years ago

    Hello Joshua, Great job in creating this NFT card component. I think their is not much area for improvement for this project but i do suggest the following:

    1. I noticed that you wrap the whole card component inside an section tag, but a more appropriate tag would be the <article></article>. Article tag are used to wrap card components.Article Tag

    2. I think you should add the style cursor: pointer; to the class .overlay of the image.

    Marked as helpful
    0
  • Angel M•370
    @csmurilloPosted almost 2 years ago

    Hello Jhonatan, I think you did a good job in this project. If you are looking for semantic tags that you could use for this project I recommend wrapping the whole card inside an <article> tag, this is used to wrap around card components. Article Tag

    Also I think you should wrap "0.041 ETH" and "3 days left" inside <p> paragraph tags. It is more appropriate, since it is more clear that it is content describing the Equilbrium card component overall.

    Marked as helpful
    1
  • Ashwin•10
    @ashwin586Submitted almost 2 years ago
    1. centering the div elements, a problem with the position.
    2. Imported font, Incorrect use of Position property.

    Product preview

    1
    Angel M•370
    @csmurilloPosted almost 2 years ago

    Hello Ashwin, Your frontend application looks similar to the solution provided. However, their is a lot of room for improvement. I made a list of things you may want to add next time around:

    1. Wrap all the main content of a particular page with the semantic tag <main></main> Main Tag

    2. Use Flexbox when you want to center things. I look through your code and saw that you used:

    .wrapper {
    ...
    position: absolute;
    top: 50%;
    left: 50%;
    -ms-transform: translate(-50%, -50%);
    transform: translate(-50%, -50%);
}

    you should instead use:

    .wrapper{
    ...
    display:flex;
    justify-content:center;
    align-items:center;
}

    this is the common method to center content, it is easier and more simpler than what you coded.

    3. Stop using margin to position content. Instead use flex box because when you create frontend web application you always want to conserve the web page content flow. Margin should only be used to separate content from one div to the other which is different to using margin to position where content should be in a webpage. When to Use Margin.)

    Flexbox works as follows, when you use display:flex; on a parent container, its children flow side by side. That said:

    In your code you can add flex on the .container:

    .container{
    display:flex;
}

    Also if you want to fix your project use this styles.css: This is your styles.css I just removed all the margin and added flex box where needed and fixed the size of the webpage using width:100%; height:100%; inside html,body. However I still left for you to add padding and fix the content where needed.

    @import url('https://fonts.googleapis.com/css2?family=Montserrat:wght@500;700&display=swap');
@import url('https://fonts.googleapis.com/css2?family=Fraunces:opsz,[email protected],700&display=swap');
/* font-family: 'Fraunces', serif; */
* {
    margin: 0;
    padding: 0;
}

html,body{
    background-image: url("vecteezy_banner-template-background-geometri-for-background_12921076.jpg");
    background-size: cover;
    width:100%; height: 100%;
    font-family: 'Montserrat', sans-serif;
}

.wrapper {
    background-color: #ffefd8 ;
    /* width: 90vw; */
    /* height: 80vh; */
    width: 100%; height: 100%;
    display:flex;
    align-items: center;
    justify-content: center;
    box-shadow: 0 4px 8px 0 rgba(0, 0, 0, 0.2), 0 6px 20px 0 rgba(0, 0, 0, 0.19);
}

.container {
    width: 550px;
    height: 450px;
    background-color: #FFFFFF;
    border-radius: 10px; 
    display: flex;
}

img {
   width: 275px;
   height: 450px;
   border-top-left-radius: 10px;
   border-bottom-left-radius: 10px;

}

p {
    font-family:'CustomFont', Montserrat;
    color: #696969;
    font-size: 14px;
    font-weight: 500;
}

h1 {
    font-family: 'CustomFont', Montserrat;
    font-weight: 500, 700;
}

#para {
    color: #696969;
    
}

#price {
    color: #18A558;
    margin-top: 30px;
    font-family: 'Montserrat';
}

#old-price {

}

button {
    width: 215px;
    height: 40px;
    background-color: #18A558;
    border-radius: 8px;
    border: 1px solid #18A558;
    cursor: pointer;
    color: #FFFFFF;
    padding: auto;
}

svg {
    padding: auto;
    margin-right: 12px;
}

    More Stuff that may be useful:

    1. Mobile First Approach Mobile First Articles

    2. Alt In Images: I would suggest reading this article Image Alt

    3. PX vs REM: I would suggest reading this Pixels Vs Rem

    IMPORTED FONT PS: The way you imported the font is wrong. Always remember to select the font weight and add font-family: 'Montserrat', sans-serif; or font-family: 'Fraunces', serif; to the div/ or whatever tag that requires it.

    If you have any questions just reply to this message.

    Marked as helpful
    0
  • Elvis•220
    @ElvislexSubmitted almost 2 years ago

    i only did this with little knowledge from html and css

    css flex

    1
    Angel M•370
    @csmurilloPosted almost 2 years ago

    Hello Elvis, I think you are a beginner, I was also once a beginner so I know. Here are my recommendations:

    1. Learn and follow the Mobile First Approach, this will help you so much when building web applications. DONT START BY BUILDING LARGE TO small screens. why? because when you actually build a website you have to be organized and it is more time efficient

    2. Learn semantic HTML. Learn when to use <header>, <nav>, <main>, and more.

    3. Learn to use CssGrid, this is used when certain position of content is to complicated to style with flexbox.

    Now for my two recommendations for this project:

    1. The Header <header> I did not see this tag anywhere in your project. But typically this is to wrap the top most content of the page. Typically a logo and links, hamburger menus.

    For this project the header should wrap the W and the links. Which in your case would be:

    <header>
          <h1>W</h1>
          <h3 class="menu>...</h3>
</header>

    2. The Menu <h3 class="menu">...</h3> should be

    <nav>
          <ul>
                    <li><a>Home</a></li>
                    <li><a>New</a></li>
                    <li><a>Popular</a></li>
                    ...
          </ul>
</nav>

    The <nav> tag is used for to wrap links, and the <ul> is widely used with nav tag since it is a list of links.

    0
  • Alexander•170
    @Perr0fe0Submitted almost 2 years ago

    Hola a todos!.

    Aquí esta mi solución a esta actividad! .

    Todo tipo de feedback se agradece.

    Testimonial grid

    2
    Angel M•370
    @csmurilloPosted almost 2 years ago

    Hola Alexander, Primer de todo muy buen trabajo modificando el css grid para hacer este projecto. Pero algunas cosas que vi en tu HTML me parece que puedes modificar para que este project quede al 100%.

    1. Puedes rodear todo el contenido de cada tarjeta testimonial en un tag <article>, este tag es muy usado para contener contenido de tarjetas. (Article Tag MDN)

    2. Otra cosa que me parece que puedes modificar es el <h1>(usando tu ejemplo):(Hgroup Tag MDN)

     <h1 class=" modo-oscuro">
          Daniel Clifford <br>
          <span class=" modo-oscuro">Verified Graduate</span>
</h1>

    un formato mas adecuado es:

    <hgroup> <h1>Daniel Clifford</h1> <p>Verified Graduate</p> </hgroup>

    La manera que lo implementaste esta bien y es muy usado en el web, pero usando el html5 tag <hgroup> es mas appropiado.

    PS: No tienes link de github para ver to codigo, si alguien mas quiere ver tu codigo y darte mas recomendaciones no va a poder ayudarte. Yo use el google dev tools.

    Marked as helpful
    1
  • Rafael Costa•30
    @rafacostadevSubmitted over 2 years ago

    I found difficult to centralize de container with the content, want to know how to do that in the right way. I need to know if my HTML code is on the right way it should be writed. I'm unsure about my CSS code, I think I could've wrote less code. I want to know how can I get my entire code better. And sorry for my bad english.

    Responsive layout using css grid, added an effect of hover on sign up.

    1
    Angel M•370
    @csmurilloPosted over 2 years ago

    The way you centered the box is the right way and widely used. In my perspective the CSS code you wrote is good. However I would suggest you use the main tag to wrap the box content.

    Marked as helpful
    1
  • Marta Dias•40
    @MDias04Submitted over 2 years ago

    I still find it very difficult to position certain elements. And get the project to be completely responsive. I think my media queries could improve.

    If anyone has some code reviews please send them my way. I really want to improve.

    Responsive coming soon page

    1
    Angel M•370
    @csmurilloPosted over 2 years ago

    Hello, if you replace @media screen and (max-width: 375px){} with @media (max-width: 576px) {} your layout will look good in mobile, since currently in some mobile devices your website looks the same as the desktop version. Other than that I think you are good with what you have accomplished for this project. However, I think there is many improvements you can make, and I would suggest when you find time, start this project all over or start another project if you don't want to repeat this project with these suggestion in mind:

    Sematic HTML: Try to include all the necessary semantic html, in the webpages you create. Like for this project I would recommend using the header tag, and the main tag. header tag: use it to wrap the logo, and the main tag to wrap the rest of the content in the page.

    Mobile First Approach: Create the styles for mobile first, then as you create styles for larger screens like tablets, desktop use media queries to target those devices. Example: tablet use this @media (min-width: 768px) {}, desktop use this @media (min-width: 992px) {} and more. I base my media queries from bootstrap breakpoints. I don't recommend targeting mobile styles with media queries.

    Alt In Images: I would suggest reading this article https://blog.hubspot.com/marketing/image-alt-text

    I think once you do more challenges in frontend mentor this might be helpful

    PX vs REM: I would suggest reading this https://uxdesign.cc/why-designers-should-move-from-px-to-rem-and-how-to-do-that-in-figma-c0ea23e07a15

    Marked as helpful
    1