-
Notifications
You must be signed in to change notification settings - Fork 17
feat : to-do #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat : to-do #39
Conversation
Thank you for your excellent work on the Todo component! The code is very clean and well-structured. I appreciate your effort in extending the component to handle child elements as well. Here's my review with some suggestions for improvement: Suggestions for ImprovementP2: Checkbox StylingThe current implementation uses a native input checkbox. While this works, it has some limitations:
Suggestion: Implement explicit styling for the checkbox to ensure consistent appearance across browsers and compatibility with Tailwind CSS projects. Our goal is to replicate Notion's design as closely as possible. P2: Notion-like DesignAs we're creating base components to mimic Notion's UI, we should aim for a design that's as close to Notion's as possible. Suggestion: Please refer to my closed PR for design inspiration, or directly reference Notion's UI to create a more accurate visual representation. P4: Component CustomizationWhile users can already customize this component, they currently need to rewrite all elements of the Todo component to do so. Suggestion: In Future Consideration (not required for this PR)To enhance customization options, consider making subcomponents like the checkbox replaceable. This could work similar to slots in Vue or Svelte, where users can optionally provide their own checkbox component (e.g., from shadcn or headless-ui), defaulting to our implementation if not specified. This level of customization isn't necessary for the current PR but is something we'll look into for future iterations of the component. Overall, excellent work! These suggestions aim to improve consistency, design accuracy, and future extensibility. Please focus on the P2 items for this PR. Let me know if you have any questions or need clarification on any point. |
Request for Review: New Requirement Added to Existing PR @Moon-DaeSeung I've implemented a new requirement in the existing pull request #39. I'd appreciate if someone could review the changes and confirm that they meet the new requirements. ![]() Changes made include
Please let me know if you need any additional information or if you have any questions. |
Thank you so much for implementing the suggestion! P1: I noticed that the border-radius seems a bit different from the original Notion design. Could you please adjust it to match the original? P2: It looks like the check mark inside the checkbox isn't centered. Could you please ensure it's properly aligned? refer to the screenshot below |
Hi @jiji-hoon96 , |
![]() Hi @Moon-DaeSeung , @myjeong19 |
please check border radius you should not use input element for checkbox considering cross brouswer |
I think you'd better respect the repository that is mentioned on contributing guide |
Thank you for your effort in submitting the PR. After careful consideration, I’ve decided to close this PR as the current implementation doesn’t fully meet the project’s needs. I believe it would be best to give another contributor a chance to tackle this issue. I truly appreciate your time and understanding, and I hope we can collaborate on other opportunities in the future. |
Description of Changes
Support 'To-do' block
Todo
component inlib/components/to-do.tsx
TodoList
component to properly structure multiple To-do itemslib/components/index.ts
to export the new To-do componentlib/index.css
Review point
To reproduce
Screenshot
Review Guide
Reviews are conducted based on priority levels, such as p0, p1, p2, p3, p4, and p5.
p0 ~ p2: If the author decides not to reflect a review for p0 to p2, it signals that a proper discussion with the reviewer is necessary. It is expected that the review will be resolved either through incorporating the feedback or through further discussion.
p3: indicates that the reviewer has identified a significant issue, but either lacks a clear solution or the comment lacks sufficient context. Further explanation or additional discussion on the reviewer's concerns is needed.
p4, p5: p4 and p5 suggest low priority, and if the author does not deem them important, these comments can be disregarded.