-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Added ui feedback when adding type in typesync #512
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
base: main
Are you sure you want to change the base?
feat: Added ui feedback when adding type in typesync #512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate you taking this task on!
Few things:
- make sure to format and lint your code
- I am not sure the typeRefs map and useEffect is the best way to scroll the newly added type into view. could just add the scroll into view in the
KnowledgeGraphBrowser.typeSelected
instead. much less state to manage and theuseEffect
has me really worried
@cmwhited thank you for your feedback. I will work on it. |
@cmwhited |
what errors is it giving you? |
I have fixed the issue in linting. I was having a package installation issue. I have updated the code. Can you review and give some feedback @cmwhited |
// Create a ref to store DOM references for each type | ||
const typeRefs = useRef<Map<number, HTMLDivElement>>(new Map()); | ||
// State to track the index of the newly added type | ||
const [_newTypeIndex, setNewTypeIndex] = useState<number | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aren't reading the state variable here: _newTypeIndex
, which I am assuming we are not because you prepended it with an underscore, then why do we need to set the state at all? It isn't being used, is it?
@@ -113,6 +113,12 @@ function SchemaBuilderComponent() { | |||
}); | |||
}, | |||
}); | |||
|
|||
// Create a ref to store DOM references for each type | |||
const typeRefs = useRef<Map<number, HTMLDivElement>>(new Map()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still does not feel like the right answer to me. Especially not the way it is being set
UI Feedback when you added a new type from the left panel
Closes #498