@Marcofa87Submitted 7 months ago
P
Srijan Manandhar
@srijanssAll comments
- P@srijanssPosted 7 months ago
- You have used landmark components like main and footer , however using article or section for card element would've been more semantic. Level 1 heading is missing for you page, so you can change h2 to h1 in your card
- There are two links in the solution, but they were not accessible using keyboard. You can change the Heading and author name to link so that it takes default focus on keyboard Tab press
- The solution differs a bit from the design, with some padding and margin fixes it will match the design
- I followed BEM methodology to define CSS classes for this solution, its really helpful to write and manage modular CSS
Marked as helpful1 - P@Biggboss7Submitted over 1 year agoP@srijanssPosted 7 months ago
Your solution looks very good, layout looks good on all screen sizes, code is well structured, readable and reusable
- You can improve the accessibility using different aria-attributes and roles to announce the game result, turns etc to screen reader users
- some minor styles and some features are missing, like, Restart game ? modal is missing
0 - P@vcollins1Submitted 8 months agoWhat are you most proud of, and what would you do differently next time?
Finishing the challenge.
What challenges did you encounter, and how did you overcome them?Ran into some issues using grid but got through it with trail and error.
What specific areas of your project would you like help with?Any feedback would be helpful.
P@srijanssPosted 8 months ago- Solution includes semantic HTML, however it would be meaningful to add main heading and form component in main element instead of header landmark
- solution is accessible using keyboard, but it moves focus to result element and some other element on the page as well which can be bit confusing
- Layout looks good on all the screen sizes
- code is well-structured, readable and reusable
- solution differs a bit from design, some styles for headings are different, and on page load welcome message should be shown instead of the bmi result information
- really nice solution , keep up the good work
0 - @konradbaczykSubmitted 8 months agoWhat are you most proud of, and what would you do differently next time?
Next time I will save the data of products in cart in variables for easier reusable.
P@srijanssPosted 8 months ago- The solution includes semantic HTML
- Solution is not fully accessible using keyboard, you can enable outline on a, button tags so that keyboard users knows about the active element on the page
- also it would be helpful for screen readers users by adding different aria attributes to define different states in the app e.g popup open/close , add to cart success etc.
- Layout looks good on all screen sizes
- Code is readable, but it can be structured into smaller components for reuse
- Solution differs a bit from the design, the lightbox component is not working
- I liked how you've used animations in menu and slider library for the image gallery
Marked as helpful0 - @kaoutar-ouadihSubmitted 10 months agoWhat are you most proud of, and what would you do differently next time?
I'm proud of completing this challenge!
What challenges did you encounter, and how did you overcome them?.
What specific areas of your project would you like help with?Anything that can help me improve.
P@srijanssPosted 8 months ago- header landmark is used properly with nav and links within it, however the nav content is duplicated for desktop and mobile view. You can use CSS to make one nav content work for both mobile and desktop
- main element is used for the content, however the headings are not used properly. heading order goes from h2 to h4
- rather than using div for the content , you can look for section or article landmark element. These elements are accessibility friendly and gives purpose for the wrapped content
- layout looks good on all the screen sizes
- code is well-structured, readable and reusable
- solution differs a bit from the design, like the menu drawer in mobile devices is not full height, on large desktop screen content spans full width. With minor changes you can make your solution match the design
0 - @samir-DeveSubmitted 8 months agoWhat are you most proud of, and what would you do differently next time?
...
What challenges did you encounter, and how did you overcome them?I did not face any challenge, it was fun to code this project. I coded it a little bit different than original design, so keep it in mind when reviewing the code and design :)
What specific areas of your project would you like help with?Any suggestion is appreciated !
P@srijanssPosted 8 months ago- <fieldset> element is used to group the inputs but the legend is missing for some fieldset. <legend> element must be added inside fieldset so its announced to screen reader user what the group is about.
- similarly there are two header tags in one page, so there is the possibility of confusion on these headers
- all the elements on the page are not using keyboard, specially the radio buttons have no outline when focused
- layout looks good on all range of screen sizes
- coudn't access the github solution page, so didn't get the chance to review the code
0 - @samir-DeveSubmitted 8 months agoWhat are you most proud of, and what would you do differently next time?
...
What challenges did you encounter, and how did you overcome them?...
What specific areas of your project would you like help with?...
P@srijanssPosted 8 months ago- The solution doesn't include semantic HTML. There is no h1 tag for the page. For all the headings h2 is used. I liked the way you have used header and main element which will separate the content and header elements.
- your solution is not keyboard accessible. You have used div to create each faq accordion item. You can used <details> element instead which is keyboard accessible by default. And if you use that element you don't need javascript to open or close the accordion.
- Layout is little bit different than the design. You are missing some of the paddings and background styles
- Code is well structured, readable. But it can be better structured for reusability
- solution doesn't differ that much from the design. If you add the missing styles, then it will perfectly match the design
0 - @GhraviteeSubmitted over 1 year agoP@srijanssPosted 8 months ago
Your solution looks nice and I like the way you've used different react components for the cards. However, you can make your app more accessible to all the users.
- You can make use of semantic HTML tags like
<article>for cards,<figure>and<figcaption>for image,<button>tag instead of<a>as this is submit button for form. web.dev's Welcom to learn A11y is very helpful regarding the accessiblity and its importance - I tried accessing the app using keyboard and only button was accessible using keyboard. You can use radio buttons for the rating items which will make it accessible using keyboard on all the browsers.
- Your solution looks good on desktop. But it overflows on mobile devices.
- Your code is well structured , readable and reusable.
- Solution is almost similar to design, except some hover states of buttons and rating items.
1 - You can make use of semantic HTML tags like
- @jrgenwebSubmitted 11 months agoWhat are you most proud of, and what would you do differently next time?
I managed to do it
What challenges did you encounter, and how did you overcome them?the dark mode, we can't even switch between the modes by clicking the switch button
What specific areas of your project would you like help with?how it turned out overall, what else should I improve
P@srijanssPosted 9 months ago- Your solution includes semantic HTML
- Keyboard navigation is not working, you can look into tabindex attribute or keydown event for different interactive elements in your app
- Layout looks good on different screen sizes, except some mid breakpoints where layout squeezes too much. You can look into CSS grid and flexbox layout for easy layout of components. And also you can use breakpoints like 550px for mobile, 768px for tablet and 1200px for desktop in this case as content width is 1160px.
- You have used Vue3 to build this app, so the code is nicely structured , readable and resuable
- Some of the layout and fonts doesn't match with the design, so the solution differ a bit from the actual design
Good work.
0 - @semperprimumSubmitted about 1 year agoP@srijanssPosted 11 months ago
Really nice solution. I will definitely refer to your code for future projects. I really liked how you've used TypeScript and how you've applied CSS to input type range and checkboxes.
I will definitely try future projects using Typescript and JS framework
1 - @fdmartiSubmitted over 1 year agoP@srijanssPosted 11 months ago
Good solution. I can see that you've used Vue.js. Really nice solution.
- I can see that you are missing some CSS for some components, thats why your solution is bit different from the design
- CSS for buttons, input:focus, input:hover, font-size for resulting tip amount and total amount etc. are different in your solution
- I think you've missed the form validation, so input error messages are not showing
- another thing I noticed is, to calculate the tip, I always needed to click the tip button. Even if the tip button is already selected I needed to click on it again to calculate the tip and total amount person
- And the calculation for total amount per person is also not correct
Hope this helps.
0 - @frontend-enSubmitted 12 months agoWhat are you most proud of, and what would you do differently next time?
I'm most proud of getting the closest method right in JavaScript.
What challenges did you encounter, and how did you overcome them?I was faced with the fact that I was thinking about how best to display cards with data on the screen and filter them, and in the end I chose insertAdjacentHTML
What specific areas of your project would you like help with?I would like to work with code performance
P@srijanssPosted 11 months agoNice solution.
-
It was very clever to use insertAdjacentHTML to render the cards
-
I will definitely refer to how you have written SCSS for styles, I will try to use it for my next project
-
The solution worked in Chrome. But somehow it didn't show the cards in my Safari. There is some error loading data.json in Safari.
-
And some hover and focus effect on cards were also missing
Happy coding!!
Marked as helpful0 -
- @chardlargodizimoSubmitted about 1 year agoWhat are you most proud of, and what would you do differently next time?
As a career shifter in front-end development, I'm proud of adding validation on the forms using JavaScript.
What challenges did you encounter, and how did you overcome them?Validation of email addresses with certain conditions. I overcome them by reading about the regex in JavaScript.
What specific areas of your project would you like help with?Improve and acquire techniques in form validations.
P@srijanssPosted 11 months agoGreat work. I like how you used regex for email validation.
Regarding the html elements. It would've been semantically correct to use <picture> element and use sources for different screen sizes. It's neat way to use multiple images for different screen sizes. And we don't need to use media queries in CSS for this one.
0 - @leannekeenanSubmitted 11 months agoWhat are you most proud of, and what would you do differently next time?
So, the instructions clearly stated HTML, CSS, and JavaScript needed to be used to complete this assignment, but I was able to complete it with just CSS and HTML. I do notice there are issues with adjusting the size of the tooltip to the container that could be addressed with JavaScript that I simply have not thought of yet. I'll revisit this assignment and rebuild it by trying to use JavaScript and see if that will help fix these issues.
What challenges did you encounter, and how did you overcome them?I had issues getting the tooltip to resize and present at the bottom of the container rather than over the share button, but used absolute positioning to get the top, left, and right of the tooltip to stretch across the width of the container, and use z-index to layer the container, tooltip, and share button to be visible/hidden as needed.
What specific areas of your project would you like help with?All help is welcome but as previously mentioned, I built this without JavaScript and therefore did not complete it according to the directions. Any guidance on how to create a JavaScript-featured solution would be greatly appreciated.
P@srijanssPosted 11 months agoYou have done really great job to complete this task without the JS. I was also thinking about using just CSS to handle the popups. But used JS just to try some querySelectors and event handling.
- You html document should have been more semantically correct. <section> element should have heading element as the child. <picture> element is more useful when you've multiple images for different screen sizes and you want the browser to select the images based on screen size. But in this case there is only one image for card-header. So, simple <img> element would've been sufficient.
- You have left the alt tag empty for all the images. This will create some accessibility issue
- You've missed some CSS styles for the active state of the design. Like the share icon styles changes in active state. To make it work I directly used <svg> image in html and changed its style using JS events like click, keydown and touchstart. I think it can also be done directly in CSS. I will try it next time.
Congrats for completing the challenge.
0 - @brainkaSubmitted 11 months agoWhat are you most proud of, and what would you do differently next time?
I am proud to have finally finished this project.
Next time, I would just approach the project in a more simplified manner and try not to overthink every single step...go with what you know!!!
What challenges did you encounter, and how did you overcome them?Many!
I was trying to incorporate both grid and flexbox, but in a totally overkill way.
Next time just prepare the html and apply either grid of flexbox where appropriate and do not try to just make it work!
Use the tool that best suits in solving the solution.
I overcame the issues but stepping back and looking at the solution again.
What specific areas of your project would you like help with?My css is in shambles with this one...
I will be working hard on this for the next project to make sure that it is more legible and to try and not repeat myself so much in order to 'just make it work'.
P@srijanssPosted 11 months agoCongrats on completing the challenge
- It looks good on all the desktop, tablet and mobile size.
- However, if we choose device size between mobile and tablet, desktop and tablet then there is issue with responsiveness. Mostly images in hero section has issues in those screen sizes.
- I can see that you have used position: relative and left: px Css properties to match with the provided design. I also used the same properties to lay out the contents on my first try. But it was too much tweaking of pixels to match the design. So, I used object-fit and object-position CSS properties for image. You can follow this link https://web.dev/learn/design/responsive-images to get all the info about responsive images.
- You have used <section>, <header> element to make the html semantically correct. But use of <header> element at multiple places can created accessibility concern. You can use <section> for the content which have header, content and have standalone meaning. You can follow this link https://web.dev/learn/html/headings-and-sections to get info about headings and sections.
You have done a great job.
0 - @natebe4Submitted 12 months agoWhat are you most proud of, and what would you do differently next time?
I was able to code this out pretty quickly, from what i understand anyway.
What challenges did you encounter, and how did you overcome them?Trying to get the button set up properly. I ended up using negative margin, im going to try and learn more to see how to do that properly. i did mis-label things at the beginning, after i was done doing the css i realized this, so i made small changes, which meant i had to alter some. but end of the day, i made it work.
P@srijanssPosted 11 months ago- Desktop layout looks really good and I liked the way you used design to write html using different div and left, right classes. It was easy to understand.
- However, mobile layout was not working. If you use media queries to change the layout and images then your mobile layout will also look good.
0 - @sumedhakorangaSubmitted 11 months agoWhat are you most proud of, and what would you do differently next time?
I used semantic HTML tags.
What challenges did you encounter, and how did you overcome them?The problem I faced was with styling.
What specific areas of your project would you like help with?I want to focus on CSS, Things are not looking 100% perfect. What should I use to learn CSS and how should I practice? Please suggest any documentation or any tool to help me with this.
P@srijanssPosted 11 months agoI really liked how you used the semantic HTML elements. For learning I usually go to this site https://developer.mozilla.org/en-US/docs/Web/CSS. It's really helpful and you get to learn about the basics and advanced stuff from there. And it's regularly updated.
0 - @slackwareeSubmitted 12 months agoWhat are you most proud of, and what would you do differently next time?P@srijanssPosted 12 months ago
I liked how you have used minimum html elements in your solution. You should add some semantic HTML elements like <article> , <ul> etc. I highly recommend this site Mozilla developer network. I regularly use this as a reference.
1
