Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

All Comments

  • P

    @miranlegin

    Submitted

    Hi all,

    this challenge was a fun one and i'm gonna try and explain my process.

    I've used Astro on this project for building .html files. I know this is kinda overkill for this challenge but i'm lazy with syncing individual .html files especially repeating sections like footer etc...

    Next i decided to create a custom map on Location page with Mapbox as i have some experience with it. It can be improved for sure but for this example i think it looks ok. I also added custom Pin from design and added Popup functionality as well.

    For layout i've mostly used CSS grid with some Flexbox along the way where needed.

    Images are marked with Picture element with multiple Source's for responsiveness. I've also added @2x versions where needed.

    Lastly i've played with :hover state on buttons with @media(hover) to only target supported devices.

    Let me know what you think!

    Cheers, Miran

    P
    Dave 5,065

    @dwhenson

    Posted

    Hi Miran - another lovely and beautiful job here!!

    Well done on using Astro, I'm keen to try and have a go at using it one day. I love the transitions you've added to the buttons, very nice!

    I've very little I can add or suggest here. One thing is that when my mouse is over the map and I scroll the map zooms in and out, and I'd prefer to just scroll - this is especially the case on smaller screens where the map takes up more of the screen (but this is really a small issue).

    On a much more general level, you might like to have a look at some of the approaches set out in every-layout.dev. Some of it is pretty advanced CSS layout stuff, but I don't think it will be too difficult for you! I love the approach to a more 'intrinsic design' approach, and it reduces a lot of the need for media queries.

    Here's my solution of the same challenge that uses some of these approaches: https://www.frontendmentor.io/solutions/art-gallery-page-more-intrinsic-design-ESLbwbiu3t (my solution is no way as nice as yours, but I think I pretty much avoided break points completely in this challenge to create a more 'fluid' solution).

    Obviously, this just depends on what kind of approach you'd like to use for CSS, but I've come to really like this way of doing things.

    Keep up the lovely work! Very inspiring for me!

    Cheers Dave

    1

  • @VasileCosmin

    Submitted

    This is my solution. I don't know why it doesn't give new advice everytime I click the button, but gives the same advice. Can you tell me what I did wrong? Thank you:)

    P
    Dave 5,065

    @dwhenson

    Posted

    Hi Vaslie

    I just did a quick test and it seems to be working fine for me! Nice job. Here's a couple of points to consider:

    1. For the button, you should probably add an aria-label to help people with screen readers understand what the button does
    2. If you really want to help them I would also add an aria-live="polite" to your h1 as this will announce the text when it is rendered to screen readers too.

    Otherwise I think this looks good. I like the way you included some default text for people to read while the first fetch is happening, and that you included a catch in case of errors.

    For the return of data I typically do something like response.ok ? response.json : new Error. This also checks the returned data is fine, and adds another level of robustness to the app.

    Hope this helps and nice work!

    Cheers Dave

    Marked as helpful

    0

  • Gabriela 70

    @ga-bri-ela

    Submitted

    Hi everyone!

    This was my first time working with an API independently (without following a tutorial). It was quite straightforward and really helped me build confidence to take on bigger challenges with APIs.

    My biggest struggle was with the positioning of the button. I am not really happy with how I solved it, and I will look into other solutions to find something more efficient. If you have any advice, I am happy to hear it.

    Any feedback is welcomed and appreciated. Thank you in advance!

    P
    Dave 5,065

    @dwhenson

    Posted

    Hello Gabriella,

    Lovely job here! In response to your button challenge here is what I would do:

    1. Remove the fixed height from the square-background. I would suggest in general to avoid setting heights if possible and use content, and if needed padding, to create a larger element.
    2. On the button, remove the absolute positioning, and add transform: translateY(50%), and that should do it.

    Transforms to position an element are great as they don't impact performance, and unusually for CSS, the percentage refers to the element being targeted, not its parent.

    So in this case we are telling the element to move down 50% of its height. Which is about where you want it I think? This also avoids hard coding pixel values which can make responsive design pretty tricky.

    Hope this helps and nice work on the fetch call. Nice that you got some JS working!

    Cheers Dave

    Marked as helpful

    1

  • Joanna Lee 130

    @doleetos

    Submitted

    Holy moly, this took quite some time to finish with ReactJS. But I had SO much fun! Please, please leave me any comments or feedback to better my code as I'm still new to React. I'm going to continue working on this to add more styles and make the site more interactive.

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey Joanna

    Lovely job here! The page looks great and responds well. I'm also learning React at the moment, but you are clearly way beyond me!

    Only a couple of small comments: as per the HTML warning I would put the testimonials section inside the main element, and probably make that sections heading a h2 (generally only one h1 page is advised).

    The only other improvement I could possibly suggest is to use pseudo-elements for the link underlines? This will give you a bit more control over shape and positioning (I am working on this challenge at the moment and used this approach - but not using React!)

    Lovely job and hope this helps a little.

    Cheers Dave

    Marked as helpful

    1

  • Goran 230

    @GoranK89

    Submitted

    This was a fun one to explore async functions and work with JSON data. The columns are dynamically drawn based on the JSON file, and the active column is also based on the current day. Any suggestions are always very welcome. 🙂

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey Goran, lovely job here! The component looks great and renders really fast which is great. Here's some points you might like to consider:

    • Heading order: In HTML we shouldn't skip heading levels (which I don't think you have done) or use heading levels for presentation purposes (which I think you have done? Using a h2 to make the price font larger for example).

    This is important as many people using assistive tech to access your pages will navigate the site based on the heading structure. At the moment this wouldn’t work with your HTML. The would probably need to skip to "my balance" for example, before the actual number.

    I approach this by first laying out the page using only HTML and only thinking about the document structure, not design at all, and then once done, I return to the page and use CSS to make things look how they should.

    We shouldn’t use headings to make text look BIG or bold. Use them only to set out your document's heading and show the document structure, and then change things up with CSS after that. It's an easy thing to do if you start out with this mindset, and can really help people.

    • getElementsByClassName is perfectly valid, but using class names to select elements has really bitten me a few times. They change so often, and get toggled on and off and all sorts of things. I'd suggest perhaps using an id or even a custom data- attribute to hook into the HTML. I do this just as standard now as using classes has given me too many headaches!

    Here's a nice article on the approach: https://gomakethings.com/strategies-for-working-with-data-attributes-in-vanilla-javascript/

    My only other small suggestions would be to include some error handling with a catch statement in your async functions, and maybe check response.ok is true before continuing. These will both just improve things when things don't work. This property is the best one to check when receiving data (as I understand it!)

    I hope this helps and bit and keep up the good work!!

    Cheers Dave

    Marked as helpful

    0

  • @mdrizwanfk

    Submitted

    I feel I have extensively and conveniently used Flexbox across this project.

    Q1. Can any flexbox alternatives be more efficient in any of the scenarios? (Below is the list of places I used flexbox:

    1. Alignment of Discounted and MRP prices.
    2. Within the Call-to-action Button for the Icon and Text's Alignment.
    3. In Desktop Design, Centering the Card Component vertically and horizontally.
    4. In Desktop Design, Flexing Image and Description within card to Row direction.
    5. In Desktop Design, Spacing a Column Flex of Card Description elements.)

    Q2. Suggestions/Improvements to structure responsive bits in the code. (style.css)

    Q3. I have not used img HTML elements for the Card image throughout this project, I used background-image CSS Rule instead, Will this impact SEO and Search results? And is my implementation considered as best practice in the industry?

    Q4. Is there a more elegant way to Round corners of our divs, in this case. I had to repeat border-radius at 2 places, one for image div and the other for the card. Can this be improved?

    Thanks for reviewing, in advance. Stay safe & take care :)

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey Rizwan, lovely job on this one, the component looks great and responds well. 🥳

    Here's some general points to consider:

    • Check the headings issue in the accessibility report. What this means is that you should use the <h1> headings as the single, main heading of your web page, followed by the <h2> headings, then the less important <h3> headings, and so on, without skipping a level.

    This is important as many people using assistive tech to access your pages will navigate the site based on the heading structure. At the moment this wouldn’t work with your HTML as you skip and level and use a h4 (presumably as it's smaller?).

    • I'd add a hover state on the button, and change the curser to pointer. I think strictly speaking we shouldn't but it's kinda expected behaviour now.

    • The entering of your component appears to be in a media query? As such at smaller widths it pops up to the top of the viewport. I'd suggest you just keep the body with display flex throughout (I often just use grid and place-items: center).

    With regard your questions,

    Q1 - You could perhaps inline the svg and then use display: inline-flex; just saves one network request, but not a big deal!

    Q2 - This can be a bit tricky, but I would try and do without the breakpoint to change from columns to row flex direction, and use flex-wrap and a min-width on either the the preview or details elements. Check out Every Layout by Andy Bell and Heydon Pickering for more o this and how far you can take flex box!

    Q3 - I would probably use a img on this one as there's no text over the image, and it is directly related to the text. I'm not sure about accessibility and CSS background images, but the img approach works. Also saves having an empty div, which is always a bit of a warning to me.

    Q4 - You could use a utility class for this perhaps? The other option could be to set the radius as a custom property? That way you can at least adjust the size easily, if not the application!

    Hope this helps a bit, lovely job over all!

    Cheers Dave

    Marked as helpful

    1

  • ania 180

    @ania221B

    Submitted

    /* Remove list styles on ul, ol elements with a list role, which suggests default styling will be removed */
    ul[role='list'],
    ol[role='list'] {
      list-style: none;
    }
    

    Yet setting role to list is treated as a mistake in the report. Is it a bad idea to use role="list" to remove default styling?

    • There was no tablet view. It doesn't seem necessary but should it be added somehow anyway?
    P
    Dave 5,065

    @dwhenson

    Posted

    Hi, just to add that there's a "bug " or rather a decision made by Safari that removes some of the functionality from lists that screen readers need, details here: https://www.scottohara.me/blog/2019/01/12/lists-and-safari.html

    I use this reset, and just accept that the HTML linter will flag issues each time. This is a bit of a tricky one, and I don't know how to call it, so in the absence of better guidance, I just follow Andy's suggestion and add the attribute.

    Marked as helpful

    1

  • P
    Dave 5,065

    @dwhenson

    Posted

    Hi @nikoescobal - lovely job here! 🥳

    I think if you add the following rules to the footer you should get the result you want, I just tried it in the browser and it seems to work (perhaps you were missing the background-position part?):

        background-repeat: no-repeat;
        background-position: center top;  
        background-size: cover;
    

    Otherwise, One thing that's worth considering is whether an element is actually a button or a link . The most important thing is what the element will do not what it looks like. This page has a great summary and lots of useful links on this: https://css-tricks.com/buttons-vs-links/.

    I would think in this case the "download" could be a button, but the other "What is it" element should perhaps be a link? All this is debatable of course, the main thing is to be aware of the different use cases.

    Keep up the great work!

    Cheers 👋

    Dave

    Marked as helpful

    1

  • Gareth 350

    @Gareth-Moore

    Submitted

    So this wasn't very difficult. But I'd love to know what you think of the API process I used and any advice regarding the JS/API.

    Also... this website works perfectly in Chrome, but in Firefox it returns the same advice even if I close and reopen the browser. It's a bit odd and almost impossible to find any solutions to fix it. If you do know, I'd love to hear about it.

    Thanks, Gareth

    P
    Dave 5,065

    @dwhenson

    Posted

    Lovely job here. The app works well 🙌

    I would just suggest in your JS to add a check to ensure the API has responded correctly and render an error message if not. This isn't too tricky, and I would just change the first then to be something like:

      .then(response => response.ok ? response.json() : Promise.reject("API failed")```
    

    If you then add a catch at the end of your fetch function you can render some fallback text in case things don't work for any reason.

    It's not a big deal, but thinking about how things can go wrong is a good habit to get into.

    Cheers 👋

    Dave

    Marked as helpful

    1

  • DanMo 120

    @DannyMolina

    Submitted

    Someone who can guide me, I managed to make the design for mobile phones but when it comes to making the desktop I get disorganized I don't know where to arrange them use grid and it's a disaster hahaha someone who can help me

    Edit: I see that it disorganized me in the naming of the classes and I assigned a class that harms everything, I do not want to throw more head at this so that is how I am crooked

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey @DannyMolina - nice work here! The page looks good. I use a few tricks to deal with the issues on your page, it's worth finding an approach that works on these points as they come up a lot! Here's what I would to do improve things:

    1. Prevent things spreading out too wide on large screens.

    There are many ways to do this but I set a grid on the body element, with three columns, as using a class selector as follows:

    .center-content {
    	display: grid;
    	grid-template-columns: minmax(1rem, 1fr) minmax(375px, 1440px)minmax(1rem, 1fr);
    }
    
    .center-content > *  {
    	grid-column: 2;
    }
    

    The 1440px is the max-width you want the main content to be, and the 1rem values is the smallest spacing you want either side of the main content on small screens (I sometimes put this to 0 and use a container to add padding to each section).

    The second part positions all direct children of the body in this nicely sized middle column. In my case, mostly, my header, main, and footer the middle column, and stops them going wider than 1440px. It’s also pretty easy to ‘break’ elements out of this constraint if you need to.

    Other people use container classes to do the same thing. This article has a good run down of alternative approaches https://css-tricks.com/the-inside-problem/ You will note I am actually using the approach the author doesn't like! I am thinking of switching to the container approach.

    Either way it's a good idea to find an approach that works for you as you'll need this for a lot of FEM challenges.

    1. Change the alignment of the buttons.

    This can be done with flexbox. Firstly I would wrap the input and button in a div. And set the following on the div:

    display: flex;
    flex-wrap: wrap;
    gap: 1rem;
    

    I would then apply the following to the input:

    flex-basis: 0;
    flex-grow: 999;
    min-inline-size: 70%
    

    This combined with the flex-wrap on the parent means that the input will grow and shrink, but when the width hits 70% the button will wrap below. Then on the button add:

    flex-grow: 1;
    min-inline-size: 20rem;
    

    This means the button will not be less than the size set (you could also use fit-content if you are feeling fancy!) I think that should work. You may need to play around with the min-inline sizes.

    1. Lastly you should add accessible name to the links listed in the report. This is probably most easily done by using aria-label. You can't have empty links with no accessible name as screen readers won't announce where the link will go.

    Hope this helps!

    Cheers

    Dave

    PS If that flexbox stuff is a bit tricky, you could always use a media query and just change the flex-direction from row to column at narrow widths.

    0

  • P
    Dave 5,065

    @dwhenson

    Posted

    Hey @Gareth-Moore lovely job here! 🥳 The component looks great and I don't have much to add! Just a couple of small things to think about:

    1. Is that a button or a link? I'm not sure of the answer, but as long as you are not deciding based on how it looks you are good! This page has a great summary and lots of useful links on this: https://css-tricks.com/buttons-vs-links/

    2. Either way, it might be nice to practice adding some hover styles to the button/link, and adding "pointer: cursor". The last point is a bit controversial for a button, but I think most sites add it. Both of these are just to give the user some hints that they can click.

    But as above, small point and lovely job! Keep it up and good luck with the next one!

    Cheers 👋

    Dave

    Marked as helpful

    0

  • P

    @mdubelbeis

    Submitted

    This project offered some challenges. Most of the challenges were self-inflicted but I was able to find work arounds to get the task complete. I will continue to correct these as time goes on.

    One of the areas I am not happy about is the responsive images and not being able to implement srcSet. This will be one of my goals because this seems to be one of the regular issues I have.

    Sunnyside-agency-landing-page

    #react#tailwind-css

    1

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey @mdubelbeis - I think you need to remove overflow-y: hidden from the body! Otherwise, it's looking good from my quick review!

    Cheers

    Dave

    1

  • @Drougnov

    Submitted

    I need some help on these issues:

    • What is the best approach/way to make the background pattern pixel perfect as shown on the design?
    • The z-index is not working on the 'quote' in the review section. How to fix it?
    • Is there a way to animate the bottom part (when visible) with only JavaScript? Or do I need some libraries/frameworks?

    I tried to make it close to the design. Yet I might miss out on many things or do wrong in some parts. Please let me know if you notice. Thanks in advance :)

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey @Drougnov lovely job here! The page looks great!! 🥳. Here's some things to think about:

    • Personally, I never worry too much about pixel perfection. I go for "pixel pretty close" here's a great article about this: https://www.joshwcomeau.com/css/pixel-perfection/

    • The quote should be on top of the review section? It is on my screen - can you clarify the issue there? Sorry if I'm missing something obvious!

    • And again for the animation - which 'bottom part' would you like to animate? You can do most things without a library, but in some cases it might be a lot of work!

    Lastly, you might want to think about stopping the page from spreading too wide a very large screens. There are many ways to do this but I set a grid on the body element, with three columns, using a class selector as follows:

    .center-content {
    	display: grid;
    	grid-template-columns: minmax(1rem, 1fr) minmax(375px, 1440px)minmax(1rem, 1fr);
    }
    
    .center-content > *  {
    	grid-column: 2;
    }
    

    The 1440px is the max-width you want the main content to be, and the 1rem value is the smallest spacing you want on either side of the main content on small screens (I sometimes put this to 0 and use a container to add padding to each section).

    The second part positions all direct children of the body in this nicely sized middle column. In my case, mostly, my header, main, and footer are the middle column and stops them from going wider than 1440px. It’s also pretty easy to ‘break’ elements out of this constraint if you need to.

    Other people use container classes to do the same thing. This article has a good run down of alternative approaches https://css-tricks.com/the-inside-problem/ You will note I am actually using the approach the author doesn't like!

    Either way, it's a good idea to find an approach that works for you as you'll need this for a lot of FEM challenges.

    Cheers

    Dave

    Marked as helpful

    1

  • @BhekiAccion

    Submitted

    Dear coders, kindly advice me here, how do l change the color of an SVG image? You will notice that l struggled to change the colour of the logo picture at the footer section. Anyone with knowledge or an informative link?

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey @BhekiAccion, nice work here!

    So to manipulate SVGs you have to 'inline' them in the HTML document. That is, actually paste the SVG as code. From there you will be able to target the different elements inside the SVG itself, if you add an SVG as an image as you have, then it's impossible to change the colour on hover etc.

    It can look a bit intimidating the first time you do this, but it's worth getting comfortable with doing this and targeting the relevant elements inside the SVG itself. It's also better for performance so you get a bonus of doing things this way. I find it helpful to think of the SVG as a document in itself.

    Hope this helps,

    Cheers

    Dave

    Marked as helpful

    1

  • P
    Dave 5,065

    @dwhenson

    Posted

    Lovely job here! The page looks good and responds well....

    You might want to think about stopping the page from spreading too wide a very large screens. There are many ways to do this but I set a grid on the body element, with three columns, as using a class selector as follows:

    .center-content {
    	display: grid;
    	grid-template-columns: minmax(1rem, 1fr) minmax(375px, 1440px)minmax(1rem, 1fr);
    }
    
    .center-content > *  {
    	grid-column: 2;
    }
    

    The 1440px is the max-width you want the main content to be, and the 1rem values is the smallest spacing you want either side of the main content on small screens (I sometimes put this to 0 and use a container to add padding to each section).

    The second part positions all direct children of the body in this nicely sized middle column. In my case, mostly, my header, main, and footer the middle column, and stops them going wider than 1440px. It’s also pretty easy to ‘break’ elements out of this constraint if you need to.

    Other people use container classes to do the same thing. This article has a good run down of alternative approaches https://css-tricks.com/the-inside-problem/ You will note I am actually using the approach the author doesn't like!

    Either way it's a good idea to find an approach that works for you as you'll need this for a lot of FEM challenges.

    Otherwise, I can see the challenge you are facing with the alignment of the error message on the CTA. One solution could be to wrap the input and the error message in a parent and use this as the basis for positioning them relative to each other?

    Again, this is a common pattern in many FEM challenges so it's worth spending a bit of time working out a way that works for you and you can apply in the future.

    Hope all that makes sense, but let me know if anything isn't clear!

    Cheers

    Dave

    1

  • P
    Anna Leigh 5,135

    @brasspetals

    Submitted

    Hi, everyone! 👋 It feels great to finally submit this solution. I’ve wanted to do this challenge since I started doing Frontend Mentor projects. It was definitely a learning experience (see: README), and hopefully I’ve made up for the lack of effects in my Sunnyside solution. 😆

    The parallax effect is only on desktop and tablet. It’s also turned off, along with all animations and page transitions, for those who prefer reduced motion. 👍

    Questions:

    • This was my first time implementing a “tabbed” interface. I opted to use button elements for the tabs. Is there a better and/or more accessible way to create tab functionality?
    • I’m concerned that the image animations on the tabbed Events section are getting messed up if the image doesn’t load before the animation triggers. Is there a way to preload images from picture elements in a way that’s responsive and also works with multiple srcsets? Update: I refactored this section a bit, making some tweaks to the animations and putting all content in the DOM rather than dynamically loaded. The flashing issue is now fixed. Huge thanks to Dave and Christopher for their help on this! 🙏
    • How is the form accessibility on the booking page? I don’t believe the second page was included in the report.
    • Sadly, I didn't manage to style the AM/PM selector like the design because I used select and option elements. Do you know of a better method that would be both accessible and match the dropdown in the design?
    • Not sure how I feel about the menu list being parallax on desktop. Does it look too wonky or have I just been staring at it for too long? 😂

    Thanks for taking the time to look at my solution! Feedback is always greatly appreciated. 😄

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey Anna - lovely job as always.

    This is my go to resources for tabbed interfaces: https://inclusive-components.design/tabbed-interfaces/ - as ever with Heydon, very in depth.

    When finding this I also saw this link which might be of interest? (I don't know Svelte so not sure how good it is.) https://github.com/aral/heydons-inclusive-tabbed-interface-in-svelte

    One small issue on the tabs is that I sometimes get a flash of the previously rendered text or photo when selecting different options. Just a small point though.

    Cheers Dave

    Marked as helpful

    2

  • P
    Dave 5,065

    @dwhenson

    Posted

    Hey Anas - lovely work on this challenge - it is quite a tricky one!! 🙌

    I would suggest the following improvements could be made to your JS;

    • I would include some input validation to check the value entered is a URL. At the moment I can try and enter other values and I don't get an indication that this isn't allowed - this will provide some quick and instant feedback to the user
    • On successful return of the API call, I would also suggest reverting the input to an empty state - just a nice thing to do for users
    • In the API call, I would include a catch statement to collect any errors - these can then be shown to the user. This is in case the URL submitted was in the proper format, but for some reason, the response didn't work out.

    If you check the response in the dev tools you can see that the response from this API has some useful error messages that can be shown to the user.

    Hope this helps a little!

    Cheers Dave

    Marked as helpful

    0

  • Jeth0214 255

    @Jeth0214

    Submitted

    Hello Guys, Is it good to have this project as my portfolio? I am just going to change those images and edit the descriptions and links. Can you guys give ideas on what I need to improve or change if I will going to use it as my portfolio.? Also, please have a code review on my solution. It will help me write my code more efficient and readable. Thanks.

    Minimalist Portfolio using HTML, CSS, Javascript

    #accessibility#fetch#sass/scss#semantic-ui#solid-js

    3

    P
    Dave 5,065

    @dwhenson

    Posted

    hey @Jeth0214, the site looks good!

    Definitely look into using a HTML solution for managing your images! My only other suggestion is to perhaps put a min-height on the body as in the contact page your footer is no longer at the bottom of the page which looks odd.

    I actually adapted this challenge for my only site, it still need work, but you can have a look here - https://dwhenson.com/ you might get some ideas of changes you could make to give it your own style.

    Cheers Dave

    Marked as helpful

    1

  • P

    @GrzywN

    Submitted

    I improved my previous solution by improving almost everything. Now with figma and better experience I was able to create pixel perfect or almost pixel perfect solution to this challenge and I'm really proud of it.

    Things I included in this solution:

    • On load animations for image and text
    • QR code popup with black glassy background (with animations)
    • Better responsiveness (it's not perfect)

    To open the popup, you need to click on the image. To close it, you need to click anywhere outside the image.

    I learned a lot about animations and trasitions doing this challenge and I recommend you guys to try making more and more fancy animations in your future projects.

    Any feedback is welcome here!

    P
    Dave 5,065

    @dwhenson

    Posted

    Nice one! The solution looks great - here are a couple of small points to consider:

    • I noted that your popup is a link element. I wondered whether this should rather be a button as it triggers an action rather than moving the user to another URL?
    • Also once the popup image is open there is no way to close it using the keyboard. A little JS might help here as otherwise keyboard users will be trapped.

    Lovely job!!

    Cheers Dave

    Marked as helpful

    0

  • P
    Anna Leigh 5,135

    @brasspetals

    Submitted

    Hi, everyone! This is my first Svelte project, so I'm sure there's much to be improved upon. Still getting used to component-based structure/development, but so far I like it!

    Safari continues to plague me. In this project, the .invalid and focus styles for the input are not showing up in Safari, and I can't figure out why. Any insight into this will be greatly appreciated! The CSS mouse animation also had some serious issues in Safari, so I opted to have the original svg icon be displayed both in Safari and for those who prefer reduced motion. Solved: Turns out my Safari was out of date! 😅 Everything works in 15.4 👍 Well, almost. Keyboard tabbing doesn't seem to pick up the maker logo in the header as a link. The outlines also display oddly on the attribution links when tabbing through them. 🤦‍♀️

    The animated mouse is heavily inspired by (read: blatantly stolen from and lightly tinkered with) Ryan Mulligan's CSS Site Scroll Micro Animation. Full list of resources in the README.

    Please let me know what you think, especially if you see anything that can be improved upon. Feedback is always welcome! 😄

    P
    Dave 5,065

    @dwhenson

    Posted

    Super lovely job!! Looks great!! 🙌

    That Safari thing is a shame - I think focus it might be fixed in the latest version (15.4) but I'm still on 15.3 so I can't try it out.

    Super nice job though!

    Cheers Dave

    Marked as helpful

    1

  • P
    Dave 5,065

    @dwhenson

    Posted

    Hey, @socoolRK nice work here - here are a few suggestions you might like to consider:

    • I'm not sure why you would add min-height 150vh to the body? It adds a lot of unnecessary scrolling to the page. I would suggest just adding max-height:100vh.
    • If you do the above and want to center things, in this case, I would suggest wrapping all the content in a div or maybe a section and then using display grid or flex on the body to put this new only child element in the center of the page - this is pretty handy for quite a few FEM challenges so it's worth finding an approach that works for you (I tend to use: display:grid; place-items:center;.
    • Just a note on the use of headings... We shouldn’t use headings to make text look BIG or bold. Use them only to set out your document's heading and show the document structure, and then change things up with CSS after that. This is important as many people using assistive tech to access your pages will navigate the site based on the heading structure. At the moment this wouldn’t work with your HTML.

    In this case, for example, you have an h2 followed by h4 (which probably shouldn't really be a heading but just a p with some styling). Most people suggest that there should only be one h1 per page so your first h2 might be better as an h1 in this case. I approach this by first laying out the page using only HTML and only thinking about the document structure, not design at all, and then once done, I return to the page and use CSS to make things look how they should.

    Again, this will come up over and over again in FEM challenges so it's worth getting an approach to this that works for you down that you can apply in other challenges too.

    But, the component looks great! And nice job using the ul for the list - a lot of people miss this! Keep up the good work.

    Cheers Dave

    1

  • renko7 50

    @renko7

    Submitted

    Hello!

    I difficulty in organizing my class names within the HTML and CSS. I am still not sure if I should be giving everything individual classes or making heavy use of selectors. If you can tell me the best practice that would be highly appreciated. As for example for my queryselectors was I supposed to just use one class? Also for my CSS was I supposed to just use once class?

    I also had a question about the light grey background that was behind the rating numbers and the star. All of the colors that I tried from the style guide were not working like that! So maybe the background is being set some other way? You will see that I just went with the dark background in the end.

    Thanks ALOT for your help!

    P
    Dave 5,065

    @dwhenson

    Posted

    Hi renko7 - regarding your question on CSS and class names, frustratingly, the answer is: it depends! This is something I struggled with for a long time, and I was worried that I wasn't doing things the "right" way. I know it's dull advice, but if you keep building things and learning and trying different approaches you will find something that works for you.

    For example, many people love Tailwind CSS, which I'd also like to try one day, which applies classes inline to each individual element, other people like a BEM approach, which moves towards removing the complications of the cascade. Personally, I love, CUBE CSS, which embraces the cascade and maps nicely to how I think about CSS.

    But don't worry about trying all these things, or even if you haven't heard of them, for the moment if you are just getting started just enjoy building things, getting stuck, finding solutions, and asking for advice when needed. It took me quite a while to settle on an approach to CSS that works for me, and it's always changing.

    Kevin Powell's responsive layout course that is free on Scrimba is a good place to move to once you have the hang of basic selectors.

    Here are a few comments with regard to your code:

    • I would check whether an element is actually a button or a link. The most important thing is what the element will do not what it looks like. This page has a great summary and lots of useful links on this: https://css-tricks.com/buttons-vs-links/ (I think you have this wrong that the moment!)
    • Good choice with the radio buttons, to make things nicer could you add a transition on the hover to make the color change a little less abrupt? And I would suggest adding the curser:pointer to the CSS for this element - not everyone would agree, but I think it's kind of expected for this type of interaction.
    • Lastly on the JS I would advise against using class names as the basis for selecting elements. This has caused me so much headache in the past, as when you start adding and removing classes the JS stops working! I try to use data- attributes when possible as I know that I can keep them just for this purpose.

    Good job and keep up the good work!

    Cheers Dave

    Marked as helpful

    2

  • P
    Dave 5,065

    @dwhenson

    Posted

    Hey nice, job here!

    To make things faster or appear faster, I would 1) move the script to the head, and add the async attribute, 2) put some placeholder text in the HTML that way if things are slow or JS fails the user still has something to read 3) not include window.onload and just execute the function as its read (you may need to swap async to defer if that messes up interactivity).

    Also, I'd suggest having another look at your fonts - they don't seem to be showing up, and also adding some fallback text in your catch so that the user still sees something if JS fails for any reason.

    Hope this helps a little

    Cheers Dave

    Marked as helpful

    0

  • Gareth 350

    @Gareth-Moore

    Submitted

    Thanks for taking a look at my project!

    I would like some feedback if you'd be so kind. Particularly, those pesky z-index problems I have when using opacity and creating transitions with overlapping elements. I don't seem to catch the drift and frustratingly I drift off into the sunset without a clue as to why my svg is not displaying even though I have opacity set to 1...? I managed to fix it but I think that was down to dumb luck.

    Thanks again! Gareth

    P
    Dave 5,065

    @dwhenson

    Posted

    Hey Gareth - Nice job here! Everything works and it looks good - that's the main thing!! 🥳

    z-indexes and opacity can get a little complicated, as elements with opacity less than one creates a new stacking context. I'm not sure if you've come across stacking contexts yet, and if you are just getting started with CSS I wouldn't worry too much, but if z-index is behaving strangely that's most commonly the case.

    If you google I am sure you can find some nice explanations of z-index and opacity and the issues it can cause.

    But, IMO, better to avoid z-indexes if you can - I think in this case just putting the SVG after the image in the HTML might work? That way you can just use the flow of the HTML document to make the SVG appear on top of the image rather than setting it manually. I tried in my dev tools quickly and I think this should work - but I only tried quickly...

    Hope this helps a little and keep up the good work!

    Cheers Dave

    1