@Marcofa87Submitted 2 months ago
P
Srijan Manandhar
@srijanssAll comments
- @srijanssPosted 2 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 - @Biggboss7Submitted 11 months ago
đ Exciting News! đ
I'm thrilled to share that I've just completed my latest project, a Tic-Tac-Toe game! đšī¸đ This classic game is not just about X's and O's; it's a culmination of effort, creativity, and a lot of coding hours.
đ Now, I'm reaching out to the amazing developer community for some collaborative fun! I would greatly appreciate it if you could take some time to review my Tic-Tac-Toe game. Let's make it even better together! đ¤đ
đ Found a bug? Have suggestions for improvement? I'm all ears! Your feedback is invaluable to me, and I'm eager to refine the game based on your insights. Dive into the code, test the game, and let me know your thoughts. Every contribution, no matter how small, makes a significant impact.
đŠâđģ Here's the GitHub repository link for you to explore the code (https://github.com/Biggboss7/tic-tac-toe). Feel free to submit issues, pull requests, or drop your thoughts in the discussions. Let's build a fantastic Tic-Tac-Toe experience together! đâ¨
Thank you all for your support and collaboration! đ Let's squash those bugs and make this game the best it can be! đŽđ§ #OpenSource #TicTacToe #CommunityCollaboration
@srijanssPosted 2 months agoYour 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 - @vcollins1Submitted 3 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.
@srijanssPosted 3 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 3 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.
@srijanssPosted 3 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 5 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.
@srijanssPosted 3 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 4 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 !
@srijanssPosted 3 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 4 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?...
@srijanssPosted 3 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 ago@srijanssPosted 3 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 7 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
@srijanssPosted 4 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 9 months ago
I would greatly appreciate any feedback you may have!
Built with React, TypeScript and SCSS
It's actually my first time using TypeScript in a project :)
Some concerns i have:
- I think the ModifierForm.tsx is a little bit too long?
- Maybe i should've used a state manager?
- How does my TS code looks overall?
@srijanssPosted 6 months agoReally 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 about 1 year ago@srijanssPosted 6 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 7 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
@srijanssPosted 6 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 -
