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

feat: Only show other recordings if there are no pinned ones by default #18915

Merged
merged 11 commits into from
Nov 28, 2023
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified frontend/__snapshots__/scenes-app-insights--retention.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2023-11-28 at 08 38 29

You could have a tooltip on the show button "Show other recordings that match these filters"

Which makes me wonder about showing that at all when there are no filters set (as in the screenshot)

But, the UX around pinning vs saving filters vs etc is maybe something to push into the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think it should always be there (you might want to just see recordings regardless of the filters).

And yeah potentially there is a completely different setup where you would want to have different "modes" of displaying.

This would all be very easy in a Notebook - more of a pain with the existing Saved Playlists concept...

Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ function RecordingsLists(): JSX.Element {
showAdvancedFilters,
hasAdvancedFilters,
logicProps,
showOtherRecordings,
recordingsCount,
} = useValues(sessionRecordingsPlaylistLogic)
const {
setSelectedRecordingId,
Expand All @@ -84,6 +86,7 @@ function RecordingsLists(): JSX.Element {
setShowSettings,
resetFilters,
setShowAdvancedFilters,
toggleShowOtherRecordings,
} = useActions(sessionRecordingsPlaylistLogic)

const onRecordingClick = (recording: SessionRecordingType): void => {
Expand Down Expand Up @@ -139,17 +142,15 @@ function RecordingsLists(): JSX.Element {
placement="bottom"
title={
<>
Showing {otherRecordings.length + pinnedRecordings.length} results.
Showing {recordingsCount} results.
<br />
Scrolling to the bottom or the top of the list will load older or newer
recordings respectively.
</>
}
>
<span>
<CounterBadge>
{Math.min(999, otherRecordings.length + pinnedRecordings.length)}+
</CounterBadge>
<CounterBadge>{Math.min(999, recordingsCount)}+</CounterBadge>
</span>
</Tooltip>
</span>
Expand Down Expand Up @@ -219,37 +220,46 @@ function RecordingsLists(): JSX.Element {
</div>
))}

{pinnedRecordings.length && otherRecordings.length ? (
<div className="px-3 py-2 text-muted-alt border-b uppercase font-semibold text-xs">
{pinnedRecordings.length ? (
<div className="flex justify-between items-center pl-3 pr-1 py-2 text-muted-alt border-b uppercase font-semibold text-xs">
Other recordings
<LemonButton size="xsmall" onClick={() => toggleShowOtherRecordings()}>
{showOtherRecordings ? 'Hide' : 'Show'}
</LemonButton>
</div>
) : null}

{otherRecordings.map((rec) => (
<div key={rec.id} className="border-b">
<SessionRecordingPreview
recording={rec}
onClick={() => onRecordingClick(rec)}
onPropertyClick={onPropertyClick}
isActive={activeSessionRecordingId === rec.id}
pinned={false}
/>
</div>
))}
<>
{showOtherRecordings
? otherRecordings.map((rec) => (
<div key={rec.id} className="border-b">
<SessionRecordingPreview
recording={rec}
onClick={() => onRecordingClick(rec)}
onPropertyClick={onPropertyClick}
isActive={activeSessionRecordingId === rec.id}
pinned={false}
/>
</div>
))
: null}

<div className="m-4 h-10 flex items-center justify-center gap-2 text-muted-alt">
{sessionRecordingsResponseLoading ? (
<>
<Spinner textColored /> Loading older recordings
</>
) : hasNext ? (
<LemonButton status="primary" onClick={() => maybeLoadSessionRecordings('older')}>
Load more
</LemonButton>
) : (
'No more results'
)}
</div>
<div className="m-4 h-10 flex items-center justify-center gap-2 text-muted-alt">
{!showOtherRecordings && totalFiltersCount ? (
<>Filters do not apply to pinned recordings</>
) : sessionRecordingsResponseLoading ? (
<>
<Spinner textColored /> Loading older recordings
</>
) : hasNext ? (
<LemonButton status="primary" onClick={() => maybeLoadSessionRecordings('older')}>
Load more
</LemonButton>
) : (
'No more results'
)}
</div>
</>
</ul>
) : sessionRecordingsResponseLoading ? (
<>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export function SessionRecordingsPlaylistScene(): JSX.Element {
</>
}
/>
{playlist.short_id ? (
{playlist.short_id && pinnedRecordings !== null ? (
<div className="SessionRecordingPlaylistHeightWrapper">
<SessionRecordingsPlaylist
filters={playlist.filters}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -489,5 +489,29 @@ describe('sessionRecordingsPlaylistLogic', () => {
}).toMatchValues({ totalFiltersCount: 0 })
})
})

describe('pinned playlists', () => {
it('should not show others if there are pinned recordings', () => {
logic = sessionRecordingsPlaylistLogic({
key: 'tests',
updateSearchParams: true,
pinnedRecordings: ['1234'],
})
logic.mount()

expectLogic(logic).toMatchValues({ showOtherRecordings: false })
})

it('should show others if there are no pinned recordings', () => {
logic = sessionRecordingsPlaylistLogic({
key: 'tests',
updateSearchParams: true,
pinnedRecordings: [],
})
logic.mount()

expectLogic(logic).toMatchValues({ showOtherRecordings: true })
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import equal from 'fast-deep-equal'
import { actions, afterMount, connect, kea, key, listeners, path, props, propsChanged, reducers, selectors } from 'kea'
import { loaders } from 'kea-loaders'
import { actionToUrl, router, urlToAction } from 'kea-router'
import { subscriptions } from 'kea-subscriptions'
import api from 'lib/api'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { objectClean, objectsEqual } from 'lib/utils'
Expand Down Expand Up @@ -240,6 +241,7 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
maybeLoadSessionRecordings: (direction?: 'newer' | 'older') => ({ direction }),
loadNext: true,
loadPrev: true,
toggleShowOtherRecordings: (show?: boolean) => ({ show }),
}),
propsChanged(({ actions, props }, oldProps) => {
if (!objectsEqual(props.filters, oldProps.filters)) {
Expand Down Expand Up @@ -340,6 +342,17 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
],
})),
reducers(({ props }) => ({
// If we initialise with pinned recordings then we don't show others by default
// but if we go down to 0 pinned recordings then we show others
showOtherRecordings: [
!props.pinnedRecordings?.length,
{
loadPinnedRecordingsSuccess: (state, { pinnedRecordings }) =>
pinnedRecordings.length === 0 ? true : state,
toggleShowOtherRecordings: (state, { show }) => (show === undefined ? !state : show),
},
],

unusableEventsInFilter: [
[] as string[],
{
Expand Down Expand Up @@ -654,6 +667,13 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
return [...pinnedRecordings, ...otherRecordings]
},
],

recordingsCount: [
(s) => [s.pinnedRecordings, s.otherRecordings, s.showOtherRecordings],
(pinnedRecordings, otherRecordings, showOtherRecordings): number => {
return showOtherRecordings ? otherRecordings.length + pinnedRecordings.length : pinnedRecordings.length
},
],
}),

actionToUrl(({ props, values }) => {
Expand Down Expand Up @@ -713,9 +733,19 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
}
}),

subscriptions(({ actions }) => ({
showOtherRecordings: (showOtherRecordings: boolean) => {
if (showOtherRecordings) {
actions.loadSessionRecordings()
}
},
})),

// NOTE: It is important this comes after urlToAction, as it will override the default behavior
afterMount(({ actions }) => {
actions.loadSessionRecordings()
afterMount(({ actions, values }) => {
if (values.showOtherRecordings) {
actions.loadSessionRecordings()
}
actions.loadPinnedRecordings()
}),
])