Skip to content

Conversation

jiji-hoon96
Copy link

Description of Changes

  • Added support for 'To-do' block
  • Implemented the To-do block component
  • Enhanced semantic structure and accessibility of the To-do component

Support 'To-do' block

  • Added a new Todo component in lib/components/to-do.tsx
  • Created a TodoList component to properly structure multiple To-do items
  • Updated lib/components/index.ts to export the new To-do component
  • Added CSS styles for the To-do component in lib/index.css

Review point

  • Should we integrate an existing checkbox solution or use the current custom implementation for the To-do block?
  • Are there any additional accessibility features we should consider for the To-do component?

To reproduce

  • npm run story:start
  • click 'Todo' on the Storybook left side panel

Screenshot

스크린샷 2024-09-27 오후 2 28 36

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.

@jiji-hoon96 jiji-hoon96 changed the title Feature/todo feat : to-do Sep 27, 2024
@jiji-hoon96 jiji-hoon96 mentioned this pull request Sep 27, 2024
@Moon-DaeSeung
Copy link
Contributor

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 Improvement

P2: Checkbox Styling

The current implementation uses a native input checkbox. While this works, it has some limitations:

  • Checkbox appearance varies across browsers
  • In projects using Tailwind CSS, the default checkbox styling might disappear entirely

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 Design

As 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 Customization

While users can already customize this component, they currently need to rewrite all elements of the Todo component to do so.

Suggestion: In index.ts, export the base components separately. This allows users to easily extend our provided components with additional modifications.

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.

@Moon-DaeSeung Moon-DaeSeung added the enhancement New feature or request label Sep 29, 2024
@jiji-hoon96
Copy link
Author

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.

스크린샷 2024-09-30 오후 2 32 21

Changes made include

  • Implemented custom checkbox styling using CSS to ensure cross-browser consistency and Tailwind CSS compatibility
  • Updated the component structure to better match Notion's layout
  • Added new CSS classes and styles to index.css for the Todo component
  • Modified the index.ts file to export the Todo component separately for easier customization

Please let me know if you need any additional information or if you have any questions.

@myjeong19
Copy link
Member

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

  • Current checkbox
    스크린샷 2024-09-30 오후 11 21 55

  • Notion checkbox
    스크린샷 2024-09-30 오후 11 22 02

@Moon-DaeSeung
Copy link
Contributor

Hi @jiji-hoon96 ,
Just checking in on the status of this PR. Have you had a chance to look at the requested changes? Let me know if you need any clarification or if there's anything holding you back.
Thanks!

@jiji-hoon96
Copy link
Author

스크린샷 2024-10-03 오후 1 52 54

Hi @Moon-DaeSeung , @myjeong19
I tried modifying it as you requested.
Thank you for your confirmation~!

@Moon-DaeSeung
Copy link
Contributor

Moon-DaeSeung commented Oct 3, 2024

please check border radius

you should not use input element for checkbox considering cross brouswer

@Moon-DaeSeung
Copy link
Contributor

I think you'd better respect the repository that is mentioned on contributing guide

@Moon-DaeSeung
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants