Feat. Users can freely customize the toggle icon. #89
Conversation
| @@ -1,4 +1,4 @@ | |||
| import { Notion, type Block } from "@notionpresso/react"; | |||
There was a problem hiding this comment.
p1: don't import with relative path
it is mono repo
see: workspace property on root package.json
There was a problem hiding this comment.
I have resolved this issue.
| import json from "./todo.json"; | ||
| import { Todo, TodoArgs } from "@notionpresso/react"; | ||
| import { Todo } from "../../../../core/src/lib"; | ||
| import { TodoArgs } from "../../../../core/src/lib"; |
There was a problem hiding this comment.
p1: don't import with relative path
it is mono repo
see: workspace property on root package.json
There was a problem hiding this comment.
I have resolved this issue.
| import json from "./toggle.json"; | ||
|
|
||
| import { Toggle } from "../../../../core/src/lib"; | ||
| import type { ToggleArgs } from "../../../../core/src/lib"; |
There was a problem hiding this comment.
p1: don't import with relative path
it is mono repo
see: workspace property on root package.json
There was a problem hiding this comment.
I have resolved this issue.
|
and It's new feature, you should mention FEAT not REFACTOR |
| import Component from "../../lib/Notion"; | ||
| import json from "./todo.json"; | ||
| import { Todo, TodoArgs } from "@notionpresso/react"; | ||
| import { Todo } from "@notionpresso/react"; |
There was a problem hiding this comment.
p3: unneccessary change
| <Toggle.Icon> | ||
| <div | ||
| style={{ | ||
| width: "10px", |
There was a problem hiding this comment.
p1: this toggle is not interactive
you should fix it
There was a problem hiding this comment.
In response to "p1: this toggle is not interactive; you should fix it," are we referring only to the custom toggle, or does this apply to all toggle icons?
| }); | ||
|
|
||
| if (!iconElement) { | ||
| iconElement = <div className="notion-toggle-button-arrow" />; |
There was a problem hiding this comment.
p3: you just declare default iconElement on
let iconElement = {{ default }} // instead of assigning null
| /> | ||
| className={`${open ? "notion-toggle-button-opened" : "notion-toggle-button-closed"}`} | ||
| > | ||
| {iconElement} |
There was a problem hiding this comment.
p3: dont iconElement inside it
iconElement should contains <div className={`${open ... }} />
custom icon can define its ui on opened or closed.
but It depends on 'notion-toggle-button-opened' css
There was a problem hiding this comment.
-
In response to "custom icon can define its UI on opened or closed"
does that mean it should accept two children—one for the open state and one for the closed state? -
For the default Notion toggle, there is a rotation animation based on the open/closed state, and I would like to maintain the current structure for the default toggle. Is that okay?
|
p1: you should explain your works more detail on pr title |
|
p1: you should answer all review (you just changed code not answering review or question) |
myjeong19
changed the title
Merge branch 'main' into refactor/toggle
2 participants
Description of Changes
Here is reference: #25
support 'toggle' block
Review point
-> I made the Toggle Button customizable.
-> I made the toggle button icon rotate when the toggle is opened.
-> I suggest passing separate props instead of using children as an approach to resolve issue #88
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.