feat: add and remove insight from dashboard modal #27138
Conversation
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
…Hog/posthog into feat/add-insight-to-dashboard-modal
There was a problem hiding this comment.
Hey @k11kirky, awesome job getting this started - seriously, this is such a great addition! It's kind of wild we didn’t have this feature already.
A few remarks from my end:
- Pagination in the modal does not work.
- It would be great to have an "in progress" state that disables the individual buttons, while an insight is added or removed.
- I know our UI is quite inconsistent across places, but it would be great if we'd get more consistent eventually (i.e. doesn't need to be addressed here, just wanted to share). In particular it would be nice to be closer to the "Add insight to dashboard" modal visually, e.g.
- A secondary "Add to dashboard" button instead of the primary "+".
- "Close" and "Add new insight" buttons in footer.
- Plain view vs table.
| </> | ||
| } | ||
| > | ||
| {/* <p>Add insight to dashboard {dashboard?.name}</p> */} |
There was a problem hiding this comment.
This file seems to contain a lot of duplication from the saved insights page. Can we dry up the code and at least import all the metadata from there?
There was a problem hiding this comment.
Hi @k11kirky, thanks for addressing the feedback! I gave this another look, and pagination still seems a bit broken on my end. For example the page doesn't reset when a filter is changed, is stored in the url although we use kea state and the item count is off when navigating to a second page.
Now I don't want perfect to be the enemy of good and maybe these issues already exist on the original page (I didn't check), but it looks like there is a bunch of code that was just copied over and isn't necessary for the modal either.
I'm thinking we might want to properly generalize the table from the saved insights page or come up with a custom one for the modal. What do you think?
|
|
||
| import type { addInsightToDashboardLogicType } from './addInsightToDasboardModalLogicType' | ||
|
|
||
| export const addInsightToDashboardLogic = kea<addInsightToDashboardLogicType>([ |
There was a problem hiding this comment.
nit: the file is called addInsightToDasboard**Modal**Logic, but the logic is called just addInsightToDashboardLogic
| onClick={() => { | ||
| showAddInsightToDashboardModal() | ||
| }} |
There was a problem hiding this comment.
nit
| onClick={() => { | |
| showAddInsightToDashboardModal() | |
| }} | |
| onClick={showAddInsightToDashboardModal} |
| logic: savedInsightsLogic, | ||
| } | ||
|
|
||
| export function InsightIcon({ |
There was a problem hiding this comment.
This icon can reuse the one from the saved insights page.
|
|
||
| const INSIGHTS_PER_PAGE = 15 | ||
|
|
||
| export const scene: SceneExport = { |
There was a problem hiding this comment.
There is no need for a scene export. Scenes are "routes" in our app and since this is a modal, we don't need one. Assuming this is a leftover from copying the saved insights page.
| {tab === SavedInsightsTabs.History ? ( | ||
| <ActivityLog scope={ActivityScope.INSIGHT} /> | ||
| ) : tab === SavedInsightsTabs.Alerts ? ( | ||
| <Alerts alertId={alertModalId} /> |
There was a problem hiding this comment.
This isn't necessary here.
| <> | ||
| <LemonTable | ||
| loading={insightsLoading} | ||
| columns={columns} | ||
| dataSource={insights.results} | ||
| pagination={{ | ||
| controlled: true, | ||
| currentPage: modalPage, | ||
| pageSize: INSIGHTS_PER_PAGE, | ||
| entryCount: count, | ||
| onForward: () => setModalPage(modalPage + 1), | ||
| onBackward: () => setModalPage(modalPage - 1), | ||
| }} | ||
| sorting={sorting} | ||
| onSort={(newSorting) => | ||
| setModalFilters({ | ||
| order: newSorting | ||
| ? `${newSorting.order === -1 ? '-' : ''}${newSorting.columnKey}` | ||
| : undefined, | ||
| }) | ||
| } | ||
| rowKey="id" | ||
| loadingSkeletonRows={INSIGHTS_PER_PAGE} | ||
| nouns={['insight', 'insights']} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
nit: React.Fragment isn't required any more
| <> | |
| <LemonTable | |
| loading={insightsLoading} | |
| columns={columns} | |
| dataSource={insights.results} | |
| pagination={{ | |
| controlled: true, | |
| currentPage: modalPage, | |
| pageSize: INSIGHTS_PER_PAGE, | |
| entryCount: count, | |
| onForward: () => setModalPage(modalPage + 1), | |
| onBackward: () => setModalPage(modalPage - 1), | |
| }} | |
| sorting={sorting} | |
| onSort={(newSorting) => | |
| setModalFilters({ | |
| order: newSorting | |
| ? `${newSorting.order === -1 ? '-' : ''}${newSorting.columnKey}` | |
| : undefined, | |
| }) | |
| } | |
| rowKey="id" | |
| loadingSkeletonRows={INSIGHTS_PER_PAGE} | |
| nouns={['insight', 'insights']} | |
| /> | |
| </> | |
| <LemonTable | |
| loading={insightsLoading} | |
| columns={columns} | |
| dataSource={insights.results} | |
| pagination={{ | |
| controlled: true, | |
| currentPage: modalPage, | |
| pageSize: INSIGHTS_PER_PAGE, | |
| entryCount: count, | |
| onForward: () => setModalPage(modalPage + 1), | |
| onBackward: () => setModalPage(modalPage - 1), | |
| }} | |
| sorting={sorting} | |
| onSort={(newSorting) => | |
| setModalFilters({ | |
| order: newSorting | |
| ? `${newSorting.order === -1 ? '-' : ''}${newSorting.columnKey}` | |
| : undefined, | |
| }) | |
| } | |
| rowKey="id" | |
| loadingSkeletonRows={INSIGHTS_PER_PAGE} | |
| nouns={['insight', 'insights']} | |
| /> |
k11kirky
changed the title
…Hog/posthog into feat/add-insight-to-dashboard-modal
Problem
You cannot add existing insights to a dashboard from the dashboard screen. This creates a confusing UX and likely duplicate created insights.
Changes
When you click add insight from the dashboard a new modal will pop up allowing you to select an exisitng insight or create a new one.