Skip to content
OverviewSolutions
0
Comments
8
Elir-Mahad
@Elir-Mahad

All comments

  • Elir-Mahad•100
    @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 helpful
    0
  • Elir-Mahad•100
    @Elir-MahadPosted almost 3 years ago

    Very 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
  • Elir-Mahad•100
    @Elir-MahadPosted almost 3 years ago

    Good 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
  • Elir-Mahad•100
    @Elir-MahadPosted almost 3 years ago

    Good job anthony keep it up !

    Fix the accessibility issue that's been highlighted by the frontendmentor site.

    0
  • Elir-Mahad•100
    @Elir-MahadPosted almost 3 years ago

    Hey 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:

    1. Add a file called codeStructure in the level of the App.js file.

    2. In codeStructure file i include a brief overview of how i organized my code (4 to 5 lines).

    3. 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.

    4. 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.

    5. 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 helpful
    0
  • Elir-Mahad•100
    @Elir-MahadPosted about 3 years ago

    Keep up the good work.

    First step, fix the accessibility and html issues that were highlighted in the report !!

    Marked as helpful
    1
  • Elir-Mahad•100
    @Elir-MahadPosted about 3 years ago

    Hey 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 helpful
    1
  • Yu Dan•30
    @SelfReflectiveSubmitted about 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!

    CSS Flexbox, HTML Div, HTML Basic, Max Width, Hoverstate, Positioning

    1
    Elir-Mahad•100
    @Elir-MahadPosted about 3 years ago

    Hey 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 helpful
    1