-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: saved insights settings in notebooks #17373
Conversation
@@ -48,38 +47,6 @@ | |||
display: block; | |||
padding-right: 1rem; | |||
} | |||
&.anim--enter { |
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.
These styles didn't seem to be used anywhere
const [dataKey] = useState(() => `DataNode.${uniqueKey || uniqueNodeKey}`) | ||
const [vizKey] = useState(() => `DataTable.${uniqueNodeKey}`) |
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.
Needed to revert to an earlier commit because using the same key for the viz & data meant that both representations shared view props e.g. the table was showing up in the settings panel
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
Code wise this looks fine.
Issues with the actual usage:
- Changing settings doesn't do anything?
- Open settings feels off. Maybe this is being solved in a different PR?
@benjackwhite both are known issues:
|
Problem
Insights & their settings don't always look great in Notebooks
Changes
Before
After
Before
Before
Before
How did you test this code?
By hand