Skip to content
OverviewSolutions
11
Comments
10
luwa
@luwa-star

All comments

  • luwa•325
    @luwa-starPosted about 2 years ago

    Excellent Solution Chamara! You can make use of nested routes in react-router.

    Marked as helpful
    1
  • luwa•325
    @luwa-starPosted almost 4 years ago

    Great job JAnneSo! I have a few observations:

    • First, you did not in indicate your <nav> in the header, it doesn't allow to browsers and other coders to recognise it as a nav bar except for the visual representation.
    • You should increase your font-size for the nav bar as it is small compared to other components in the <header> section.
    • Your class names are not descriptive enough. Descriptive class names help your code-readbility.
    • There is white space in your landing page in between the sections. Fixing your margin or padding property should fix that.
    • The hover effect on your Creations section is quite sharp you should consider in creasing it to probably opacity:0.8 or opacity:0.7 to ease the effect on the eyes.
    • Your <header> section completely disappears when the screen size is reduced. You should fix that. Moving on to your source code:
    • There's no need to create two CSS folders. Its increasing the size of your site, just put it in all in one folder.
    • According to css semantics, there shouldn't be anything after the <footer> tag. It's either you put your contact in the <footer> tag or you remove it. While it's not showing on your landing page it doesn't look so good on your source code that should be rectified. Overall, you did a great job, keep coding!
    0
  • nika-sergeeva•150
    @nika-sergeevaSubmitted almost 4 years ago

    Hi guys! I'd be glad to hear any comment on my work.

    I've made amends the project and reloaded files. I have several questions..

    I don't understand why in my mobile version I can't set gaps to space lines?=( It looks well in Devtools Responsive mood but it messed up when I open the site from my iphone. The same issue I have with icons at the bottom of the site (but horizontally). I tried to use relative measurements like % rem but alas.

    HTML CSS jQuery

    1
    luwa•325
    @luwa-starPosted almost 4 years ago

    Great job Marvelous! This is a great attempt at making a landing page. However I have a few observations:

    • The font-size you used for your 'quote' section is very large as opposed to that of the nav-items, there's no balance. It just keeps staring at me, lol. you might want to wrap it in <h1> tag then style it from there.

    • you need to use semantic elements like<main>, <article>,<nav> and so on in your page layout to make your a bit more readable and accessible. You can read it up.

    • Your class names are not descriptive. It makes your code quite unreadable.

    • I noticed your <a> tags don't have attributes. It makes your code inaccessible. It's bad practice not to add attribute to anchor tags at least href attributes should be there so browers can recognise it as a link.

    • Your landing page is not responsive. Use absolute units like px when you don't want the width/ height to change. The use of relative units like rem, em, % are better that way when you resize the screen it will adjust. Also, the use of media queries are very important because you can't design your pages for all screen sizes all at once. remember to start from mobile first that way you can scale up easily. The use of Devtools also helps.

    • The use of Grid. In my opinion, you overused grid. There are sections where you could have used display:flex or display:inline-block and the reposition the other elements. For example, in your home section for the navbar you could have used both display:flex and display:inline-block to position the logo and nav items. Then used position:absolute to position the the quote on the image. The same thing goes for the footer, you could have wrapped the logo and nav links in a <div> tag and social media links and footer-note in a <div> tag. Then used display:inline-block to space out. Thus avoiding using the grid layout.

    • Finally, your images for the creations Items are zoomed out when the screen size is reduced. You can fix that by using the background-size: cover or background-position:center and if there's an overflow ,you can set it to overflow:hidden. that should fix any glitch.

    Overall, this is a good attempt at making a landing page. I feel you should read up more on HTML so you can get better at making great layouts and have a better understanding of how the elements work together to achieve a particular purpose, it helps with good practices and accessibility and SEO. My two cents!! Keep coding Marvelous.

    2
  • luwa•325
    @luwa-starPosted almost 4 years ago

    Great job! It looks nice on large screens. However, it's not responsive. Each box elongates when the screen is reduced. My suggestion is that you change the layout at various breakpoints so it won't look so cramped. Also, from experience it's best to start with mobile view so you can build from there. Keep coding!

    0
  • luwa•325
    @luwa-starPosted almost 4 years ago

    This is really awesome. I absolutely love it, it's really responsive. Keep it up. The only thing I can't seem to get over is the font-size it's so big. You should reduce it at various breakpoints because it keeps standing out as opposed to the slider image. Keep coding!

    1
  • luwa•325
    @luwa-starPosted almost 4 years ago

    Great job RajMhatre20! You did a fantastic job. You might want to make it more responsive by using all the resources given in the starter file. It's not so nice seeing a pixelated picture when in reducing screen size. A way to do that is possibly using display:none to remove the mobile and desktop hero images when its not neede and you can use display:flex to change the layout. Finally, I feeel you should add a message to indicate that the email has been sent successfully. It helps the user experience. My two cents. Keep coding!

    1
  • Himanshu•20
    @himsnghSubmitted about 4 years ago

    Why my design when viewed in comparison window is messed up and when I open up in the website its fine ?? Can anyone please help me?

    HTML- CSS

    1
    luwa•325
    @luwa-starPosted about 4 years ago

    Great job!! The design is really nice.

    I have a few suggestions:

    • the design is not responsive. Using media queries should fix that. When you are designing cards avoid using width, max-width is better. That way the width can change according to the viewport.

    • the color of text is not sharp. I can hardly see it. Use opacity:0.7. For cards 3 and 5 use the color you use for the text with an opacity:0.7 to make it visible.

    • the height of cards 3 and 4 are not the same. You should fix it. My two cents🙂

    0
  • luwa•325
    @luwa-starPosted about 4 years ago

    Great job! It's really nice.

    I have a few suggestions:

    First, responsiveness, your design is not responsive at all. I suggest add Media queries so the view can be nice on all screens. Also, to improve responsiveness, use more of relative units like em, rem, %, etc. Instead of using width why not use max-width for the container. Mind you, the max-width should be set for each screen size in your media queries.

    Secondly, the you didn't change the colour of the <p> for card 3 and 5. It's had to read the text. The same goes for the names and verified graduate section. That's my two cents 🙂

    0
  • Dwight•215
    @SarsiPCSubmitted about 4 years ago

    10/21/20

    I'm having issues with stretching the background image all the way through.

    Honest feedback/criticism is appreciated! Any suggestions on how I can improve this solution?

    Update 10/21/20

    • Fixed the background issue after countless tweaks.

    background-position: left top, right bottom;

    Social proof section | FLEXBOX

    3
    luwa•325
    @luwa-starPosted about 4 years ago

    Great job! It looks really nice.

    However, it looks nice on specific screens. I suggest you make better use of the media queries. If you choose to only consider the screen sizes in the style-guide.md . Your design won't be responsive on other screen sizes. Instead of using max-width:1280px try going for min-width:1280px. Also, choose a suitable min-width for medium screen sizes.

    Secondly, for screen sizes with width less than 320px there's an offset. I suggest you check that out it might be that you are using the wrong unit or there's an extra padding somewhere. You need to check it out. Btw I don't really understand why you are using min and max within the same class. Is it that there's a concept behind it? From what I know CSS interprets code in order. Don't be offended, I genuinely don't know.

    0
  • luwa•325
    @luwa-starPosted about 4 years ago

    This is a very good attempt at making a grid. However, there are a number of issues with this design. First is responsiveness, you used absolute units, for you to have a responsive design you need to use relative units like rem, em, %, vh and vw. So when the screen is reduced or increased, it is exactly what you want it to be. Also, because of the units used there was an overflow which is completely unnecessary. For better responsiveness you can use add about two more breakpoints for medium, large and extra large screens. Second, there is a lot of negative space, the margin and padding is not so good. The grid is not centered. It's best to wrap the grid in a container then play around with margins and paddings until you get the desired effect. Third, the testimonial image is so small, you have to squint your eyes to see the face of the persons. You need to increase the image size. Still on this, just for effectiveness use border-radius:50%. This way no matter how large the image size you'd still get the rounded image you are going for.

    1