Elir-Mahad
@Elir-MahadAll comments
- @Anthony-2003Submitted almost 3 years ago@Elir-MahadPosted almost 3 years ago
Good job Anthony. Your improvement is remarkable you are doing a great job. Don't stop coding !!
Marked as helpful0 - @Anthony-2003Submitted almost 3 years ago
something to improve?
@Elir-MahadPosted almost 3 years agoVery good work anthony !!!!
Things to fix:
-
In your solution, the width of the card is larger than than the width from the design.
-
In your solution, the hover effect must be active when the mouse is hovered in the txt not out of text. In other words, when your mouse is on the word 'change', then it should become dark blue. If the mouse is not on the word change, then it should be very light.
0 -
- @Anthony-2003Submitted almost 3 years ago
Something to improve?
@Elir-MahadPosted almost 3 years agoGood job anthony keep up the good work!
Things to fix:
-
In your solution, the width of the boxes, seems to be larger than the width of the boxes in the design.
-
In your solution, the space between the text and the 'learn more' button, seems to be larger than the space in the design.
Fix these issues and make sure that your solution matches the design as much as possible.
0 -
- @Anthony-2003Submitted almost 3 years ago
Something to improve? x)
@Elir-MahadPosted almost 3 years agoGood job anthony keep it up !
Fix the accessibility issue that's been highlighted by the frontendmentor site.
0 - @xlr8torSubmitted almost 3 years ago
Whew...this one really took a lot of effort. Feedback is highly welcome
@Elir-MahadPosted almost 3 years agoHey there friend !
The site looks good.
Regarding the functionality:
Things that work:
-
When i click the checkmark of a todo item, then click on active, the items that show up in the active list are the correct ones.
-
When i remove an item by clicking on the X, the correct number of items left is shown on the bottom left of the box.
-
The dragging and dropping functionality works.
-
The dark mode works and looks beautiful !!!
-
Site is responsive !!
Issues/suggestions:
-
When i click the checkmark of a task. Then press the X .. it gets removed from the list. But then when i go to the completed tab, the completed task does not show up there.
-
If i add a new todo item, then when i click the x, the task is not removed from the list.
-
The checkmark circle, next to the create a new todo input field, seems to serve no purpose. Perhaps you can remove it.
Regarding your code:
- Consider using one Style.js file, that has all your styled components; so that in your components folder, you'll have only the js files. This is something that i do, that's why i'm suggesting it. But i also like how you organized it.
When i expect other people to read my code i usually:
-
Add a file called codeStructure in the level of the App.js file.
-
In codeStructure file i include a brief overview of how i organized my code (4 to 5 lines).
-
On top of each component j.s file, i write a few lines (2 or 3) explaining the purpose of this component and what is exactly happening in the code.
-
Some comments on your code would also be useful. For example: Line 14 of control.js is :
onClick={() => {props.setFilter('All'); setActiveBtn({active:'All'});}}>All</Button>
. When i look at this line, i wouldn't immediately know where the filter useState exists, because its not in this file. I found that it was set in the App.js line 19. Instead of me having to look for it, it would be easier if you made a comment to indicate these types of things. -
You are a one line machine. The whole process of reading your code would be a lot faster if you broke things down. For example: In ListItem.js line 11 :
props.isActive({id:props.text.id, completed:!checked, active: checked})
can be broken down into smaller pieces with comments explaining what's happening.
I assume other people, who are not at your level, will try to read your code on this site, so it would be helpful for them (i.e, me lol). I usually write things out like this:
const handleChecked = () => { // comment setChecked(!checked); // comment props.isActive({ // comment id:props.text.id, // comment completed:!checked, // comment active: checked // comment }) // } // Below is an example of how i commented on one of my own projects: const getRestaurant = (id) => { // The constant getResturant stores a process // it takes in an id as a parameter RestaurantDataService // Enter the RestaurantDataService component .get(id) // run the function called get and pass the id into it .then((response) => { // grab the response setRestaurant(response.data); // insert the response data into the constant restaurant; console.log(response.data); // console log the data }) .catch((e) => { console.log(e); }); // console.log any errors };
It might seem redundant but it expedites the reading process.
keep up the good work !
Marked as helpful0 -
- @PaienobeSubmitted almost 3 years ago
Its kind of slow but it works. Please give me feedback
@Elir-MahadPosted almost 3 years agoKeep up the good work.
First step, fix the accessibility and html issues that were highlighted in the report !!
Marked as helpful1 - @nkusikevinSubmitted almost 3 years ago
all good please if you have any Question you can ask me via github thank you
@Elir-MahadPosted almost 3 years agoHey there :-) !
You did a great job. Your code is well organized, clean, and good. I had no trouble understanding what you did and this is always a good thing.
I looked through your css and i noticed that some improvements could be made.
General feedback:
- Try to write your css in the same format of your html.
- Example: Since your attribution is at the bottom of the html, then the attribution styles should be at the bottom of css.
Specific feedback:
All of my feed back is in the code below. Here is what the abbreviations mean:
R: Stands for 'Remove'.
- You should remove this line because when i put it in a comment, it had no effect on your component.
MC: Stands for 'my comment'. Here i am talking to you directly.
body { background-color: var(--very-dark-blue-main-bg); /* R: position: relative; */ font-family: "Outfit", sans-serif; font-family: "Signika Negative", sans-serif; } /* .......Card.......... */ .card { /* R: position: absolute; */ /* R: top: 30px; */ /* R: right: 0; */ /* R: left: 0; */ /* R: display: flex; */ /* R: align-items: center; */ background-color: var(--very-dark-blue-card-bg); /* R: justify-content: center; */ /* R: flex-direction: column; */ /* R: border-radius: 10px; */ padding: 2rem; margin: 0 auto; width: 330px; height: 555px; /* MC: I added the margin-top below */ margin-top:3rem; } /* ...... Main image........ */ .card-top img { object-fit: cover; width: 100%; height: auto; border-radius: 5px; } /* .......First content .......... */ .card-content > h2 { color: white; font-size: 25px; line-height: 2; } .card-content > p { color: var(--soft-blue); font-size: 17px; } /* ...........Price................... */ .card-price { display: flex; justify-content: space-between; align-items: center; /* R: text-align: center; */ margin: 1rem 0; } /* MC: Below i combined price-left and price-right because they have the same styles. The text and image were not aligned in your version, so i aligned them using flex. */ .price-left, .price-right{ color: var(--yan); display:flex; justify-content: space-between; align-items:center; } /* MC: You should give the icon in the price-left a class and then give it margin-left:0.5rem; */ /* card-owner */ /* MC: I placed the hr here to match the flow of the design. */ hr { height: 1px; border: none; background-color: var(--very-dark-blue-line); } .card-owner { display: flex; align-items: center; margin-top: 1rem; } .card-owner img { width: 20%; border-radius: 50%; border: 1px solid wheat; margin-right: 1rem; } .card-owner p { color: var(--soft-blue); font-size: 15px; } .card-owner p span { color: white; } /* Attribution below */ /* MC: I placed the attribution here to match the flow of the design. */ .attribution { color: var(--yan); font-size: 15px; text-align: center; position: fixed; bottom: 0; right: 0; left: 0; } .attribution a { color: var(--yan); }
Keep up the good work !!
Marked as helpful1 - @SelfReflectiveSubmitted almost 3 years ago
Hello im Yu from Germany - aspiring Fullstack Web / App Developer. I´m still at the Beginning of my Journey but i have learned alot so far. This is my take on the NFT Card i felt like i did the challenge but in a very clunky way. I would LOVE to get some Feedback on my Code! I would aswell take a look at yours if you want :)
Thank you so much everyone!
@Elir-MahadPosted almost 3 years agoHey Yu i looked over your code !
It looks clean and not clunky at all.
In your website: https://selfreflective.github.io/NFT-preview-card-component/
There seems to be a horizontal and vertical scroll.
This probably because of the height and width in your body.
I played around with the css and i made some changes to body, .ntf-card, .boxshadow
Implement the below changes to the css classes and let me know what you think.
I identified the css properties that i removed by putting them under 'removed css below' comment.
I identified the css properties that i added by putting them under 'added css below' comment.
body { text-align: center; background-color: hsl(217, 54%, 11%); margin: 0; font-family: Outfit; /* removed css below */ /* width: 1440px; */ /* height: 900px; */ } .ntf-card { border-radius: 10px; background-color: hsl(216, 50%, 16%); width: 340px; /* remove css below */ /* position: relative; */ /* margin: 200px auto; */ /* Added css below */ display:block; margin:auto; margin-top:2rem; } .boxshadow { display: flex; height: 300px; width: 300px; justify-content: center; align-items: center; position: absolute; border-radius: 11px; display: flex; justify-content: center; background-color: hsl(178, 100%, 50%); /* removed css below */ /* margin-bottom: 47px; */ /* bottom: 234px; */ }
Also it would be useful, when you are writing out your code, to avoid using position relative and absolute unless its absolutely necessary.
Try using flexbox and margins to move around the page.
Thoughts ?
Marked as helpful1