-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(homepage): add default platforms module on the homepage #14942
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🔴 Meticulous spotted visual differences in 5 of 993 screens tested: view and approve differences detected. Meticulous evaluated ~9 hours of user flows against your PR. Last updated for commit 2df6fd4. This comment will update as new commits are pushed. |
Bundle ReportChanges will increase total bundle size by 2.04kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
|
||
- name: page-templates | ||
version: v3 | ||
version: v4 |
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.
need to think about this is necessary for OSS or not..
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.
let's revert this and not update the version
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.
overall looking solid, just a few questions around props to AutoCompleteEntityItem
and making sure we do send analytics for this module!
<DisplayNameHoverComponent | ||
let displayNameContent; | ||
|
||
if (customOnClick) { |
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'm a little confused why this is needed - i see EntityItem
has this and we pass customOnClick
down here, but why do both need that?
i also see we pass in onClick={sendAnalytics}
from EntityItem but that wouldn't get used if we have a custom on click right?
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.
and i do think we want to send analytics here so we know what module is being interacted with like we do for other modules
{customHover ? ( | ||
customHover(entity, <DisplayNameWrapper>{displayNameContent}</DisplayNameWrapper>) | ||
) : ( |
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.
yeah and same thing - do we need custom hover? maybe
Linear ticket:
https://linear.app/acryl-data/issue/PRD-2299/add-new-platforms-default-module-to-home-page
Description:
This PR adds a new module for showing platforms. It also adds platforms module as a default module on the homepage.
Screenshot: