-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 : Support for Announcements listing in Activity Feed Tab and Announcement Tab in Service page #18993
base: main
Are you sure you want to change the base?
Conversation
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
await expect(page.getByTestId('announcement-error')).toContainText( | ||
'No Announcements, Click on add announcement to add one.' | ||
); | ||
|
||
await page.getByTestId('add-announcement').click(); |
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 still have this for no announcement right? inside announcement tab?
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, we have, added this check in the test
if (isForActivityFeedTab) { | ||
const activityFeedText = (await page | ||
.getByText('Activity Feeds & Tasks') | ||
.isVisible()) | ||
? 'Activity Feeds & Tasks' | ||
: 'Activity Feeds'; |
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.
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.
done, used role id as the selector.
// Edit the reply message | ||
await page.hover('[data-testid="replies"] > [data-testid="main-message"]'); | ||
await page.waitForSelector('.ant-popover', { state: 'visible' }); | ||
await page.click('[data-testid="edit-message"]'); | ||
|
||
await page.fill( | ||
'[data-testid="editor-wrapper"] .ql-editor', | ||
'Reply message edited' | ||
); | ||
|
||
await page.click('[data-testid="save-button"]'); | ||
|
||
await expect( | ||
page.locator('[data-testid="replies"] [data-testid="viewer-container"]') | ||
).toHaveText('Reply message edited'); |
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 are missing this test
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.
done,Added this test
if (isForActivityFeedTab) { | ||
const activityFeedText = (await page | ||
.getByText('Activity Feeds & Tasks') | ||
.isVisible()) | ||
? 'Activity Feeds & Tasks' | ||
: 'Activity Feeds'; |
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.
same
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.
done
const callback = () => { | ||
return; | ||
}; | ||
await updateThreadData(threadId, postId, isThread, data, callback); |
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.
await updateThreadData(threadId, postId, isThread, data, callback); | |
await updateThreadData(threadId, postId, isThread, data, noop); |
can use noop
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.
used noop
<div | ||
className="d-flex justify-between" | ||
data-testid="announcement-sub-tab"> | ||
<Space align="center" size="small"> | ||
<AnnouncementsIcon | ||
style={COMMON_ICON_STYLES} | ||
{...ICON_DIMENSION} | ||
/> | ||
<span>{t('label.announcement-plural')}</span> | ||
</Space> | ||
|
||
<span> | ||
{!isUserEntity && | ||
getCountBadge( | ||
countData.data.conversationCount, | ||
'', | ||
activeTab === ActivityFeedTabs.ANNOUNCEMENTS | ||
)} | ||
</span> | ||
</div> | ||
), |
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 guess we have tabLabel component which can be used here
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, we have tabLabel component , but ig that's for the tabs ,not for the sub tabs. We can use this compo for sub tabs as well, but currently its not supporting icon display, which we require for sub tab.
margin-bottom: 5px; | ||
margin-right: 5px; |
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.
Can we use our size variable instead hardcoded CSS values?
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.
done
@@ -264,6 +264,7 @@ const UserPopOverCard: FC<Props> = ({ | |||
<Link | |||
className={classNames( | |||
'assignee-item d-flex gap-1 cursor-pointer items-center', | |||
'z-9 relative', |
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.
Avoid using z-index
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.
done
@@ -605,6 +605,7 @@ | |||
"in-lowercase": "in", | |||
"in-open-metadata": "In OpenMetadata", | |||
"in-review": "In Review", | |||
"inactive": "Inactive", |
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.
Please translate and update it with help of AI
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.
done
{ | ||
name: t('label.announcement-plural'), | ||
key: EntityTabs.ANNOUNCEMENT, | ||
count: 0, |
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 need to replace it with actual count 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.
yes, that will be replaced when the backend changes are available for total announcements count in counts api call. Currently we are not fetching all the announcements (active & inactive both) all at once. Fetching only the selected announcement type ie. active or inactive.
Quality Gate passed for 'open-metadata-ui'Issues Measures |
Note: The implementation for the count of announcements is currently not included in this PR. This will be added once the necessary backend changes are completed and available.
Announcements Listing in Activity Feed
Screen.Recording.2024-12-16.at.8.31.49.PM.2.3.1.mov
Announcements Listing in Service Page
Screen.Recording.2024-12-16.at.8.33.13.PM.1.1.1.mov
Update Manage Button
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>