-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(ui) Add framework for template/module state and editing templates/modules to UI #14029
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
base: master
Are you sure you want to change the base?
Conversation
… - working correctly with minimal testing
🔴 Meticulous spotted visual differences in 18 of 1162 screens tested: view and approve differences detected. Meticulous evaluated ~7 hours of user flows against your PR. Last updated for commit cd0b4e5. This comment will update as new commits are pushed. |
const personalTemplate = user?.settings?.homePage?.pageTemplate || null; | ||
const globalTemplate = settings.globalHomePageSettings?.defaultTemplate || null; | ||
|
||
if (!userLoaded || !globalSettingsLoaded) return null; |
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.
we'll probably want to add some loading state here later
const quickLink = { | ||
title: 'Quick Link', | ||
key: 'quick-link', | ||
label: <MenuItem description="Choose links that are important" title="Quick Link" icon="LinkSimple" />, | ||
onClick: () => { | ||
// TODO: open up modal to add a quick link | ||
}, | ||
}; | ||
|
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.
decided to hardcode these here so we can have easy onClick handlers here instead of in a constants class. open to changing this up though
} else { | ||
// Add to existing row | ||
const { rowIndex } = position; | ||
if (rowIndex >= newRows.length) { | ||
// Create new row if index is out of bounds | ||
newRows.push({ | ||
modules: [module], | ||
}); | ||
} else { | ||
const row = { ...newRows[rowIndex] }; | ||
const newModules = [...(row.modules || [])]; | ||
|
||
if (position.rowSide === 'left') { | ||
newModules.unshift(module); | ||
} else { | ||
newModules.push(module); | ||
} | ||
|
||
row.modules = newModules; | ||
newRows[rowIndex] = row; | ||
} |
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.
we can update the logic in here for adding a 4th module to the next row.
once we add more logic it probably makes sense to break all this into a helper utility function with even more tests on it
Bundle ReportChanges will increase total bundle size by 3.82kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
❌ 2 Tests Failed:
View the full list of 3 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
name, | ||
type, | ||
scope, | ||
visibility: { |
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.
I see that we don't have visibility
attribute in UpsertPageModuleInput
import { DataHubPageModuleType, EntityType, PageModuleScope } from '@types'; | ||
|
||
// Input types for the methods | ||
export interface CreateModuleInput { |
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.
I see that we already have CreateModuleInput
and AddModuleInput
in context/types.ts
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.
yes i actually fix this in a followup PR that is currently blocked from being put up by a few things. i have such a weave of PRs right now, is it cool if I just handle these duplicate types in a followup once that PR comes into plat?
className?: string; | ||
originRowIndex?: number; |
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.
It's edge case but if we get more that 3 modules in a single row we will wrap them during the rendering and rowIndex can be increased because of it. So in this case we could add module in the wrong place. Maybe it's possible to preprocess PageTemplateFragment
(global and personal) to get rows wrapped in the state before rendering instead of wrapping them during rendering. What do you think about?
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.
hmm is it okay if we handle that in a separate ticket? this is such a monster PR and so many things depend on this getting in that I'd like to handle anything like edge cases in followups if possible
This PR introduces a comprehensive framework for managing customizable page templates and modules in the DataHub UI, enabling users to dynamically add and organize widgets on their home pages.
Key Features
🏗️ Template & Module Framework
🎯 Built-in Modules
🧪 Testing & Quality
UI/UX Impact
Users can now customize their home page by adding relevant modules through a clean, organized dropdown menu. This establishes the foundation for a fully personalized DataHub experience.
Technical Notes
NOTE: the diff is deceptively high with at least 70% being tests here