Feedback is much appreciated , still learning react had some issues with the context I made the state keeps making infinite loops. I made the slider and the navbar into to components one for mobile and the other for desktop, so is this a good way or there is a a better one?
Vee
@JunasVeeAll comments
- @HussinSoliSubmitted over 2 years ago@JunasVeePosted over 2 years ago
Hi Hussin!
Your page is responsive and it looks good to me, I can't really mention all of the issues since you said you're still learning reactjs which I think it's common to have difficulties functionalizing particular things. But here are the issues that are such a "big deal" in my personal opinion:
-
The cart, it's one of the main concepts in this challenge. In Ecommerce Product design, having a cart like this makes the customers know what they have been putting on. I saw that you have set the function for it but it doesn't seem working on my device, please let me know if it actually works.
-
Increment and Decrement, I tried to decrease the number when it's 0. it's good that the output doesn't show a negative number, but if you click it 3 times after 0, in order to put it back to 0 it also takes 3 increment clicks, so behind the scene, it is still a negative value without showing it to the output. So instead of creating a string for negative values, when counter <= 0 set its value to 0 too.
I hope my feedback is helpful.
Marked as helpful1 -
- @KohseyPowerSubmitted over 2 years ago
Feedback welcome ! ^^
This is my first submition. I'm trying to learn and practice HTML and CSS. There are probably bad practises, notice me !
@JunasVeePosted over 2 years agoHi Kohsey!
I've checked your preview site and its codes in your repository. It looks awesome to me, simple codes only a few lines and the CSS is short as well but everything works fine. Additionally, it's responsive to all screen sizes. I would say that your efficiency is at another level!
Perhaps the things that you might want to fix are accessibility issues in the report but those issues aren't extremely important. Overall, absolutely well done.
Marked as helpful1 - @Kratos012Submitted over 2 years ago
- I had difficulties with font-size. Choosing the correct/appropriate font-size is extremely difficult for me.
- I also had lot's of issues with layout, what units to use, How to scale the background image to fit perfectly into a container/box . I ended up using vh for the height.
Well, I'll appreciate some feedback from you guys. Thanks I've still got a ways to go.
@JunasVeePosted over 2 years agoHi Kratos012!
Congrats on finishing this challenge, I gotta say that your solution looks clean and almost perfect. I'll kick this off by answering your inquiries:
-
I know that this challenge tells you to make the solution looks as close as possible to the example design and probably it includes the font-size, but when it comes to a real-life situation, what actually matters are its readability and its suitability with the web design. This might be a bit off the box but trust me, it would be more useful once you achieve something in this field.
-
px unit is good when a page is made for a single specific viewport and defines the height or width of an element. Most developers use Relative Units such as rem and em for font-size, vh for page's height, and vw for page's width. I personally use % for margin and padding to make it responsive. To fit this particular image, I guess it'd be better to use the img tag instead, I've tried it, and actually worked, additionally, you can make the container's width wider.
My Feedback:
- Somehow you made the image looks better and more comfortable to the eyes by blending the background color to it, great job.
- It seems like the attribution affected your main content's position by pushing it up from the center point. To give the attribution an independent position, set its position to absolute so that it won't affect any other elements.
The rest is pretty much well done. I hope my feedback is helpful.
Marked as helpful1 - @afaiz-spaceSubmitted over 2 years ago
my second challenge on front-end mentor.
@JunasVeePosted over 2 years agoHi Faizan!
Good to know that you've finished your 2nd challenge on this platform, your solution looks good, however, I've found several issues that you might want to fix. Here are the issues:
-
Flexbox, the Main Container should always be both vertically and horizontally centered but in your case, it's not vertically centered even though you've added
align-items: center;
on the main tag. The problem is, you haven't set the height of the container, therefore it makes the flexbox confused because by default it follows the child element's tallest height. To fix this, addmin-height: 100vh;
for the main tag and your issue will directly be fixed. -
Font-Family I suggest you read the style-guide.md file, it says that the font-family should be Lexend Deca but it's not implemented in your solution. To fix this, import the font from the google font website and set it as the font-family for every element.
-
Styling/CSS, We don't need to add a type attribute to a style tag because HTML already knows what it is, removing it would be better because it has no value and is making the code looks filthy. Additionally, it gives you an HTML issue report for this challenge.
-
Main tag, instead of setting a div as the main tag, you can directly use the main tag itself. As a matter of fact, it provides a better Search Engine Optimization(SEO).
There are actually a few more issues including the texts but I guess it would take too long to type. The point is to keep learning from your mistakes because it's better than not making anything.
I hope my feedback is helpful.
Marked as helpful0 -
- @dawidPoznanskiSubmitted over 2 years ago
Feel free to give me a feedback 😁.
@JunasVeePosted over 2 years agoHi Dawid!
It is great to know that you finished this challenge, your page looks nice and interesting. However, there will always be room and time for improvements. Here are the issues that I found in your solution:
-
On my screen, the text is big enough and clear, great. But when I tried to open it on bigger screen size, it looks so tiny. Perhaps you can make a media query for a viewport larger than 1800x1000 and increase the content's font-size until it is readable on large screens.
-
I saw that you haven't given your background the properties that it needs. It's duplicating or in the Web Development, we call it background repeating.The size is smaller than most desktop screen sizes as well. In the order to fix that, you can put
background-repeat: no-repeat; background-size: cover; background-position: center;
on the body tag in CSS. -
I'm not sure why, but I think there should only be 1 footer tag in each document. To fix this, either wrap the social media components in a div instead of a footer tag or delete the attribution with its footer tag. It makes better accessibility.
-
This is the biggest issue, you didn't add the mobile media query. I suggest you make one because it is really important since everyone has a mobile phone and tends to do anything from their phones.
The point of these issues is that your page isn't responsive. But it's all back to you whether you want to improve or not, I'm not forcing you, I'm just here to give you the feedback that I think I better give. Keep going 👍
Marked as helpful0 -
- @DawidMTXSubmitted over 2 years ago@JunasVeePosted over 2 years ago
Hi DawidMTX!
I saw that there's no orange box image file in your folder, so I assume that the problem is that the wanted file is missing.
Your solution works as it should, great. But it seems like it has some issues with the UI/UX or the styling. Here are the things that you might want to fix:
- If I'm not mistaken, the card's background should be white/#fff instead of grayish-blue as it shows in the design example. But it doesn't really matter since background can be changed easily anytime.
- The top, left, and right side of the
box-shadow
is too big that it makes the violet background looks really dark near the card. - In large screen sizes the text is too tiny, I suggest you increase the
font-size
a bit for both headings and p tags. - In large screen sizes the FAQs go all the way up to the top of the container instead of standing still at the center just like the design example shows. Since you're using flexbox to position your elements, you can add
align-items: center;
to the main tag in the CSS so that it'll vertically centers the components in any screen size.
You provide what the challenge wants you to provide regardless of how the output looks but improving it would be better, Great Job and keep going.
Marked as helpful0 - @remyboireSubmitted over 2 years ago
Altought it is not perfect, it has been a good way to learn a bit of algorithmic thinking. I tried to make the computer play smart and block the player's moves. CPU plays randomly, but if its opponent can win, it will block it, otherwise, if CPU can win, it will play to win. This is basic, but seems to get people into enough trouble.
Any feedback is welcome !
@JunasVeePosted over 2 years agoHi Remy!
I would say that this is fantastic! I tried to play against the comp and I couldn't win even a single round. The Front-End looks absolutely interesting. I visited the JS file and there are tons of lets but still looks clean and the logic/algorithm is out of my mind, all buttons and their functions work properly. I guess your intention to get people into enough trouble has been fulfilled as well😂👍
Additionally, no accessibility or HTML issue. Definitely 10 out of 10, you made me learn a lot from your solution.
0 - @LiliYao2022Submitted over 2 years ago
Hi there, This is my first coding challenge, actually I am a bit confused about how to set mobile size or desktop size for my design. Also, I learned about " display: flex, oh, it is so powerful, thank you!
@JunasVeePosted over 2 years agoHi Lili!
It seems like something is wrong when you uploaded your files to Github. The preview link only shows the README.md file's content. What I suggest you do is:
- Directly and only upload the exact "qr-code-component-main" folder itself or in other words the folder where the index.html file is located/placed instead of the folder that is nesting the index.html's folder with another unnecessary file.
After you implement this solution, I'm 99.9% certain that your website will work as expected.
Marked as helpful0 - @offedwardSubmitted over 2 years ago@JunasVeePosted over 2 years ago
Hi! It looks like your codes are the same as mine. They way the output works, the class and ID names, as well as the javascript codes. In fact, the JS code is 100% the same as mine! considering that I submitted the solution first, I assume you copied my code and this is against my owning rights! I suggest you delete or change this solution before I claim this as a copyright strike!
1