Todo app using ReactJs and Sortable JS

Community feedback

• 

  Miroslav•180

  @miroslavdurinPosted over 2 years ago

  Hello and congratulations on completing this solution.

  I would suggest that you add a class .scroll if a list is longer than 9, or whatever number you like. You can change container div inside of ToDoList.js to something like this:

  <div className={`container ${theme?"dark-theme":"light-theme"} ${list.length >= 9 && "scroll"}`}

  Inside Main.scss you can change to this:

  .container {
          @media (max-width:$mobile-width){
              margin-top: 20px;
          }
          margin-top: 50px;
          max-height: 500px; /* You can put any height you like */
  
          /* This class is added if list.length is bigger than 9 */
          &.scroll {
              overflow-y: scroll;
          }
  
          /* Custom Scrollbar styles */
          &::-webkit-scrollbar {
              width: 10px;
              
              cursor: pointer;
          }
            
          /* Track */
          &::-webkit-scrollbar-track {
              box-shadow: inset 0 0 5px grey; 
              border-radius: 10px;
          }
             
            /* Handle */
          &::-webkit-scrollbar-thumb {
              background: var(--light-grayishblue-hover); 
              border-radius: 10px;
              
          }
            
            /* Handle on hover */
          &::-webkit-scrollbar-thumb:hover {
              background: var(--dark-grayishblue); 
          }
      }
  

  I've added some styles for a scroll-bar too.

  There is also issue with a check mark, which is not centered. I think that inside of .todo-box img you can remove these 2 lines:

  right:2px;
  top:5px;
  

  And I've noticed that on a screen bigger than 1450px background-image is not full width. You can just add background-size: contain inside of .App class.

  Marked as helpful

  2

  ---

  

  Cường•885

  @docuong0912Posted over 2 years ago

  @miroslavdurin that's a great idea, really detail explaination, i will give it a try, thank you for your time and effort

  1
• 

  Jon Wright•160

  @Dr-Wrong-MoPosted over 2 years ago

  I see the problem with your background exceeding your content. There are a few items you need to change.

  The primary issue is that your div with the class "todo-box" has a position of absolute (I'll start referring to this as div.todo-box or just .todo-box). The property absolute position takes it out of the normal content flow. It is now positioned within the closest element that has position relative. If no ancestors have position relative, it becomes relative to the window.

  I suggest you study CSS Flex-Box and CSS Grid for your layout properties.

  The reason this is important is that your primary element (div.App) is now functionally empty.

  Position absolute should be used sparingly, and almost never on your primary content.

  Here is what I changed to make it work:

  1. On div.todo-box, remove "position: absolute"

  2. On div.App, change "height = 100vh" to "min-height = 100vh"

  This will fix the problem where your background cuts off, but it will mess up the formatting for the child elements within div.todo-box. It will also make your todo-box align to the left side of the screen.

  You can recenter your content by modyfing div.App as follows:

  display: flex; justify-content: center; padding-top: 100px;

  This will look better but your dark mode toggle will still have absolute positioning, which will move it to the top-right of the screen.

  Remove position absolute (as well as the top and right properties). This will place it directly under your heading.

  I would wrap the heading and the image in a parent div. Let's call it div.heading. So it would look like this:

  <div class="heading"> <h1>todo</h1> <img .../> </div>

  Now the CSS

  .heading { display: flex; justify-content: space-between; }

  This has almost worked, but your hidden icon is blocking the shown icon from being where it wants. This is because you used "opacity: 0" to hide it. Opacity does not remove the item from the flow of the body. It only makes it less visible, but it is still taking up space. What you can do instead is apply "display: none". This will remove it from the flow of the body, allowing your shown icon to fit where you want.

  Your icon will need to have some new properties applied to get it to have the shape you want.

  A few other notes:

  I didn't like the interface of needing to click on the checkbox in order to submit a new item. It would be better if I could press enter to submit the item, clear the input field, and place the cursor back into the now empty input field, ready for another new item.

  Also, the positioning of the checkmark within the checkboxes is off. In my previous comment, I mentioned that you should study up on Flex-Box. This is another expample of where you could use Flex-Box (or Grid).

  The project looks decent otherwise. Good job!

  Marked as helpful

  0

  ---

  

  Cường•885

  @docuong0912Posted over 2 years ago

  @Dr-Wrong-Mo omg, i violated a lot basic mistake, thanks for pointing that out for me, i did applied flex box in the checkbox but some how the check image didn't fit in.

  About the opacity, i want to make an animation when toggle the theme change button. After setting its opacity to 0 i also set the z-index to -1, which i personally think won't affect much to other elements.

  Lastly, the interface, as AngeliqueDF mentioned, i will fix it later.

  Thank you a lot for your time and effort. i really appreciate it.

  0

  

  Jon Wright•160

  @Dr-Wrong-MoPosted over 2 years ago

  @docuong0912 I see what you are saying about using opacity for animations.

  I see the problem with that. You can't change both display and opacity at the same time.

  Here is a simple solution to overcome that. You give the element two separate classes. One for display none and one for opacity 0. You toggle the display class first, set a timeout for 1ms or more, then toggle your opacity class.

  Here is an example that shows it in action. Notice that the button moves when it is clicked, because the heading is removed from the document flow.

  https://codepen.io/Dr_Wrong/pen/WNzvmKw

  Marked as helpful

  0

  

  Cường•885

  @docuong0912Posted over 2 years ago

  @Dr-Wrong-Mo thank you sir, how thoughtful of you, new knowledge acquired, i'll fix it later. Are there any problem left to solve,sir?

  0

  

  Jon Wright•160

  @Dr-Wrong-MoPosted over 2 years ago

  @docuong0912 It looks like you haven't handled the mobile design, so you could work on your media queries too. Keep it up!

  I'd love to see it again when you have polished it up more. If you don't mind, reply to one of my comments when you're done, so I can check out your finished product.

  0
• 

  Angélique D. Faye•390

  @AngeliqueDFPosted over 2 years ago

  Hi Nguyễn, this solution looks great!

  About your question, I think the best way to adapt the layout when there are a lot of tasks is to use CSS Grids for the layout.

  I would apply the background on the body element, give it a min-height: 100vh;. Then use display: grids; to create the layout. I haven't tried this yet but I think it could work.

  We have similar solutions and I didn't think of this case, so thanks!

  Small feedback: can you make it so that we can add tasks by pressing the Enter key? It might be more intuitive than pressing the button.

  Marked as helpful

  0

  ---

  

  Cường•885

  @docuong0912Posted over 2 years ago

  @AngeliqueDF i don't really understand your solution, but the feedback is helpful, thanks for your idea

  1

  

  Angélique D. Faye•390

  @AngeliqueDFPosted over 2 years ago

  @docuong0912 no problem!

  0