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

chore: open / close notebook widgets #17375

Merged
merged 25 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
923c095
chore: open / close notebook widgets
daibhin Sep 11, 2023
fbaa8d8
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 11, 2023
b306117
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 12, 2023
5411813
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 12, 2023
d5b7939
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 12, 2023
c7dade6
Update UI snapshots for `chromium` (1)
github-actions[bot] Sep 12, 2023
b7faf60
leave settings open
daibhin Sep 12, 2023
a836b78
hide if not custom node
daibhin Sep 12, 2023
6c4b3e1
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 12, 2023
bba72cb
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 13, 2023
675168a
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 13, 2023
af079ee
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 13, 2023
7e190bb
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 13, 2023
d327e6e
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 14, 2023
9d715a9
Update UI snapshots for `chromium` (1)
github-actions[bot] Sep 14, 2023
d2c7348
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 14, 2023
63b3c0c
remove widget feature flag
daibhin Sep 14, 2023
fd883c7
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 14, 2023
f2fcbd3
Update UI snapshots for `chromium` (1)
github-actions[bot] Sep 14, 2023
1ff763d
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 14, 2023
33d201d
add toggle support & active state
daibhin Sep 14, 2023
632b16e
Merge branch 'master' into dn-chore/notebooks-widgets-toggling
daibhin Sep 15, 2023
da3cc83
Update UI snapshots for `chromium` (1)
github-actions[bot] Sep 15, 2023
4067347
Update UI snapshots for `chromium` (2)
github-actions[bot] Sep 15, 2023
fefb59e
Update UI snapshots for `chromium` (1)
github-actions[bot] Sep 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 0 additions & 1 deletion frontend/src/lib/constants.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,6 @@ export const FEATURE_FLAGS = {
REFERRAL_SOURCE_SELECT: 'referral-source-select', // owner: @raquelmsmith
SURVEYS_MULTIPLE_CHOICE: 'surveys-multiple-choice', // owner: @liyiy
CS_DASHBOARDS: 'cs-dashboards', // owner: @pauldambra
NOTEBOOK_SETTINGS_WIDGETS: 'notebook-settings-widgets', // owner: #team-monitoring
PRODUCT_SPECIFIC_ONBOARDING: 'product-specific-onboarding', // owner: @raquelmsmith
REDIRECT_SIGNUPS_TO_INSTANCE: 'redirect-signups-to-instance', // owner: @raquelmsmith
APPS_AND_EXPORTS_UI: 'apps-and-exports-ui', // owner: @benjackwhite
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/lib/lemon-ui/LemonWidget/LemonWidget.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function LemonWidget({ title, collapsible = true, onClose, children }: Le
/>
</>
) : (
<span className="flex-1">{title}</span>
<span className="flex-1 text-primary-alt px-2">{title}</span>
)}
{onClose && <LemonButton status="danger" onClick={onClose} size="small" icon={<IconClose />} />}
</Header>
Expand Down
7 changes: 4 additions & 3 deletions frontend/src/scenes/notebooks/Nodes/NodeWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export function NodeWrapper<T extends CustomNotebookNodeAttributes>({
}: NodeWrapperProps<T> & NotebookNodeViewProps<T>): JSX.Element {
const mountedNotebookLogic = useMountedLogic(notebookLogic)
const { isEditable } = useValues(mountedNotebookLogic)
const { setIsShowingSidebar } = useActions(mountedNotebookLogic)

// nodeId can start null, but should then immediately be generated
const nodeId = attributes.nodeId
Expand All @@ -86,7 +87,7 @@ export function NodeWrapper<T extends CustomNotebookNodeAttributes>({
}
const nodeLogic = useMountedLogic(notebookNodeLogic(nodeLogicProps))
const { title, resizeable, expanded } = useValues(nodeLogic)
const { setExpanded, deleteNode, setWidgetsVisible } = useActions(nodeLogic)
const { setExpanded, deleteNode } = useActions(nodeLogic)

const [ref, inView] = useInView({ triggerOnce: true })
const contentRef = useRef<HTMLDivElement | null>(null)
Expand Down Expand Up @@ -165,9 +166,9 @@ export function NodeWrapper<T extends CustomNotebookNodeAttributes>({
/>
)}

{!!widgets.length && isEditable ? (
{widgets.length > 0 ? (
<LemonButton
onClick={() => setWidgetsVisible(true)}
onClick={() => setIsShowingSidebar(true)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should toggle, rather than just showing. And it would be great if it could have an "active" state if open.

size="small"
icon={<IconFilter />}
/>
Expand Down
3 changes: 1 addition & 2 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodePlaylist.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { SessionRecordingPlayer } from 'scenes/session-recordings/player/Session
import { useMemo, useState } from 'react'
import { fromParamsGivenUrl } from 'lib/utils'
import { LemonButton } from '@posthog/lemon-ui'
import { IconChevronLeft, IconSettings } from 'lib/lemon-ui/icons'
import { IconChevronLeft } from 'lib/lemon-ui/icons'
import { urls } from 'scenes/urls'
import { notebookNodeLogic } from './notebookNodeLogic'
import { JSONContent, NotebookNodeViewProps, NotebookNodeAttributeProperties } from '../Notebook/utils'
Expand Down Expand Up @@ -130,7 +130,6 @@ export const NotebookNodePlaylist = createPostHogWidgetNode<NotebookNodePlaylist
{
key: 'settings',
label: 'Settings',
icon: <IconSettings />,
Component: Settings,
},
],
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodeQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { useMemo } from 'react'
import { notebookNodeLogic } from './notebookNodeLogic'
import { NotebookNodeViewProps, NotebookNodeAttributeProperties } from '../Notebook/utils'
import clsx from 'clsx'
import { IconSettings } from 'lib/lemon-ui/icons'
import { urls } from 'scenes/urls'
import api from 'lib/api'

Expand Down Expand Up @@ -147,7 +146,6 @@ export const NotebookNodeQuery = createPostHogWidgetNode<NotebookNodeQueryAttrib
{
key: 'settings',
label: 'Settings',
icon: <IconSettings />,
Component: Settings,
},
],
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/scenes/notebooks/Nodes/NotebookNodeRecording.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
} from 'scenes/session-recordings/playlist/SessionRecordingPreview'
import { notebookNodeLogic } from './notebookNodeLogic'
import { LemonSwitch } from '@posthog/lemon-ui'
import { IconSettings } from 'lib/lemon-ui/icons'
import { JSONContent, NotebookNodeViewProps, NotebookNodeAttributeProperties } from '../Notebook/utils'

const HEIGHT = 500
Expand Down Expand Up @@ -102,7 +101,6 @@ export const NotebookNodeRecording = createPostHogWidgetNode<NotebookNodeRecordi
{
key: 'settings',
label: 'Settings',
icon: <IconSettings />,
Component: Settings,
},
],
Expand Down
11 changes: 0 additions & 11 deletions frontend/src/scenes/notebooks/Nodes/notebookNodeLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ export const notebookNodeLogic = kea<notebookNodeLogicType>([
timestamp,
sessionRecordingId,
}),
setWidgetsVisible: (visible: boolean) => ({ visible }),
setPreviousNode: (node: Node | null) => ({ node }),
setNextNode: (node: Node | null) => ({ node }),
deleteNode: true,
Expand Down Expand Up @@ -112,22 +111,12 @@ export const notebookNodeLogic = kea<notebookNodeLogicType>([
setNextNode: (_, { node }) => node,
},
],
widgetsVisible: [
false,
{
setWidgetsVisible: (_, { visible }) => visible,
},
],
})),

selectors({
notebookLogic: [(_, p) => [p.notebookLogic], (notebookLogic) => notebookLogic],
nodeAttributes: [(_, p) => [p.attributes], (nodeAttributes) => nodeAttributes],
widgets: [(_, p) => [p.widgets], (widgets) => widgets],
isShowingWidgets: [
(s, p) => [s.widgetsVisible, p.widgets],
(widgetsVisible, widgets) => !!widgets.length && widgetsVisible,
],
}),

listeners(({ actions, values, props }) => ({
Expand Down
6 changes: 1 addition & 5 deletions frontend/src/scenes/notebooks/Notebook/Notebook.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import { NotebookConflictWarning } from './NotebookConflictWarning'
import { NotebookLoadingState } from './NotebookLoadingState'
import { Editor } from './Editor'
import { EditorFocusPosition } from './utils'
import { FlaggedFeature } from 'lib/components/FlaggedFeature'
import { FEATURE_FLAGS } from 'lib/constants'
import { NotebookSidebar } from './NotebookSidebar'
import { ErrorBoundary } from '~/layout/ErrorBoundary'

Expand Down Expand Up @@ -99,9 +97,7 @@ export function Notebook({ shortId, editable = false, initialAutofocus = null }:
) : null}

<div className="flex flex-1 justify-center space-x-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this was always the case but the space-x-2 causes a gap when there are no widgets showing... (see the snapshot diff)

<FlaggedFeature flag={FEATURE_FLAGS.NOTEBOOK_SETTINGS_WIDGETS}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this because settings widgets are much more stable (and necessary now) given insights are working

<NotebookSidebar />
</FlaggedFeature>
<NotebookSidebar />
<ErrorBoundary>
<Editor
initialContent={content}
Expand Down
25 changes: 16 additions & 9 deletions frontend/src/scenes/notebooks/Notebook/NotebookSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { notebookNodeLogicType } from '../Nodes/notebookNodeLogicType'

export const NotebookSidebar = (): JSX.Element | null => {
const { selectedNodeLogic, isShowingSidebar, isEditable } = useValues(notebookLogic)
const { setIsShowingSidebar } = useActions(notebookLogic)

if (!isEditable) {
return null
Expand All @@ -17,23 +18,29 @@ export const NotebookSidebar = (): JSX.Element | null => {
'NotebookSidebar--showing': isShowingSidebar,
})}
>
<div className="NotebookSidebar__content">{selectedNodeLogic && <Widgets logic={selectedNodeLogic} />}</div>
<div className="NotebookSidebar__content">
{selectedNodeLogic && isShowingSidebar && (
<Widgets logic={selectedNodeLogic} onClose={() => setIsShowingSidebar(false)} />
)}
</div>
</div>
)
}

export const Widgets = ({ logic }: { logic: BuiltLogic<notebookNodeLogicType> }): JSX.Element | null => {
const { widgets, nodeAttributes, isShowingWidgets } = useValues(logic)
const { updateAttributes, setWidgetsVisible } = useActions(logic)

if (!isShowingWidgets) {
return null
}
export const Widgets = ({
logic,
onClose,
}: {
logic: BuiltLogic<notebookNodeLogicType>
onClose: () => void
}): JSX.Element | null => {
const { widgets, nodeAttributes } = useValues(logic)
const { updateAttributes } = useActions(logic)

return (
<div className="NotebookNodeSettings__widgets space-y-2 w-full">
{widgets.map(({ key, label, Component }) => (
<LemonWidget key={key} title={label} onClose={() => setWidgetsVisible(false)}>
<LemonWidget key={key} title={label} collapsible={false} onClose={onClose}>
<div className="NotebookNodeSettings__widgets__content">
<Component attributes={nodeAttributes} updateAttributes={updateAttributes} />
</div>
Expand Down
12 changes: 8 additions & 4 deletions frontend/src/scenes/notebooks/Notebook/notebookLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export const notebookLogic = kea<notebookLogicType>([
exportJSON: true,
showConflictWarning: true,
onUpdateEditor: true,
setIsShowingSidebar: (showing: boolean) => ({ showing }),
registerNodeLogic: (nodeLogic: BuiltLogic<notebookNodeLogicType>) => ({ nodeLogic }),
unregisterNodeLogic: (nodeLogic: BuiltLogic<notebookNodeLogicType>) => ({ nodeLogic }),
setEditable: (editable: boolean) => ({ editable }),
Expand Down Expand Up @@ -166,6 +167,13 @@ export const notebookLogic = kea<notebookLogicType>([
setEditable: (_, { editable }) => editable,
},
],
isShowingSidebar: [
false,
{
setSelectedNodeId: (showing, { selectedNodeId }) => (selectedNodeId ? showing : false),
setIsShowingSidebar: (_, { showing }) => showing,
},
],
}),
loaders(({ values, props, actions }) => ({
notebook: [
Expand Down Expand Up @@ -334,10 +342,6 @@ export const notebookLogic = kea<notebookLogicType>([
}
},
],
isShowingSidebar: [
(s) => [s.selectedNodeLogic],
(selectedNodeLogic) => selectedNodeLogic?.values.isShowingWidgets,
],
}),
sharedListeners(({ values, actions }) => ({
onNotebookChange: () => {
Expand Down
1 change: 0 additions & 1 deletion frontend/src/scenes/notebooks/Notebook/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export type NotebookNodeViewProps<T extends CustomNotebookNodeAttributes> = Omit
export type NotebookNodeWidget = {
key: string
label: string
icon: JSX.Element
// using 'any' here shouldn't be necessary but, I couldn't figure out how to set a generic on the notebookNodeLogic props
Component: ({ attributes, updateAttributes }: NotebookNodeAttributeProperties<any>) => JSX.Element
}
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading