Skip to content
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

fixes node title not changing #536

Merged
merged 6 commits into from
Nov 27, 2024
Merged

fixes node title not changing #536

merged 6 commits into from
Nov 27, 2024

Conversation

mck
Copy link
Collaborator

@mck mck commented Nov 26, 2024

Closes #484

@mck mck requested a review from SorsOps November 26, 2024 12:17
Copy link

changeset-bot bot commented Nov 26, 2024

🦋 Changeset detected

Latest commit: a70b5b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tokens-studio/graph-editor Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

Failed to generate code suggestions for PR

);

// Update local state when the annotation changes
React.useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The observer should trigger the rerender anyway, we shouldn't need to update the local state

/>
</Stack>
);
};
});

const NodeSettings = ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could actually do an observer here in the node Settings once to auto rerender the attributes here

@@ -15,7 +15,7 @@ import { observer } from 'mobx-react-lite';
import { useGraph } from '@/hooks/useGraph.js';
import { useSelector } from 'react-redux';

export function NodeSettingsPanel() {
export const NodeSettingsPanel = observer(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't do anything if there isn't an observable passed to it

},
[selectedNode],
);
const NodeDescription = observer(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need these observers if the parent NodeSettings has the observer applied ? We shouldn't as it should detect the change and rerender the children

@SorsOps SorsOps merged commit 89d2dd0 into master Nov 27, 2024
2 checks passed
@SorsOps SorsOps deleted the fix-node-title branch November 27, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node title change breaks
2 participants