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(ui) Create page for managing home page posts #8707

Conversation

chriscollins3456
Copy link
Collaborator

@chriscollins3456 chriscollins3456 commented Aug 23, 2023

First off I want to give a HUGE shoutout to @PatrickfBraz for his work that went into this PR here. He did the vast majority of this in his PR here: #8292
His work is what enabled us to push this over the finish line otherwise who knows how long it would take before we got there. So thanks so much again!

This PR creates a new page in settings where users that have the platform privilege for managing home page posts can go to in order to create and delete home page posts of either text or link kind. Right now we don't allow editing since that resolver doesn't exist yet.

This also makes some styling updates to make posts look nicer.

Screenshots:
image

image image image image

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added product PR or Issue related to the DataHub UI/UX devops PR or Issue related to DataHub backend & deployment labels Aug 23, 2023
@@ -107,7 +107,31 @@ public static boolean canEditGroupMembers(@Nonnull String groupUrnStr, @Nonnull
}

public static boolean canCreateGlobalAnnouncements(@Nonnull QueryContext context) {
return isAuthorized(context, Optional.empty(), PoliciesConfig.CREATE_GLOBAL_ANNOUNCEMENTS_PRIVILEGE);
final DisjunctivePrivilegeGroup orPrivilegeGroups = new DisjunctivePrivilegeGroup(
ImmutableList.of(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@@ -74,19 +79,21 @@ export const PostLinkCard = ({ linkPost }: Props) => {
const link = linkPost?.content?.link || '';

return (
<CardContainer type="link" href={link}>
<CardContainer type="link" href={link} target="_blank" rel="noopener noreferrer">
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you!! so much better than redirecting.

@@ -143,6 +147,11 @@ export const SettingsPage = () => {
<TeamOutlined /> <ItemTitle>Ownership Types</ItemTitle>
</Menu.Item>
)}
{showHomePagePosts && (
<Menu.Item key="posts">
Copy link
Collaborator

Choose a reason for hiding this comment

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

fire!

</SubFormItem>
</>
)}
</Form>
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks great. copy also improved!

thanks for adding the select with conditional fields

})
.catch((e) => {
message.destroy();
message.error({ content: `Failed to create Post!: \n ${e.message || ''}`, duration: 3 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: i've been trying to remove places where we print the raw backend message, and replace with a console.error + An unknown error occurred for the end user

Copy link
Collaborator

Choose a reason for hiding this comment

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

(tiny)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hear that


// Handle the Enter press
useEnterKeyListener({
querySelectorToExecuteClick: '#createPostButton',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!!

<PageContainer>
<PageHeaderContainer>
<PageTitle level={3}>Home Page Posts</PageTitle>
<Typography.Paragraph type="secondary">View and manage your DataHub Posts.</Typography.Paragraph>
Copy link
Collaborator

@jjoyce0510 jjoyce0510 Aug 23, 2023

Choose a reason for hiding this comment

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

Maybe add something about the posts appearing on the landing page.

"View and manage pinned posts that appear to all users on the landing page. "

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah so much better

export default function PostItemMenu({ title, urn, onDelete }: Props) {
const [deletePostMutation] = useDeletePostMutation();

const deleteDomain = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: deletePost

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nicee

title: 'Type',
dataIndex: '',
key: 'type',
render: (record: PostEntry) => PostColumn(record.contentType),
Copy link
Collaborator

Choose a reason for hiding this comment

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

reminder: we should make sure the copy shown aligns with what the user created (Announcement vs Link)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

definitely good call will update this

export const DESCRIPTION_FIELD_NAME = 'description';
export const LINK_FIELD_NAME = 'link';
export const LOCATION_FIELD_NAME = 'location';
export const TYPE_FIELD_NAME = 'switch';
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's called "switch"? does type not work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it does lol it used to be called SWITCH_FIELD_NAME but i changed it and forgot to update the value here (even though technically it doesn't matter cuz it's just a const that's used) - but updating!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

Overall looks great. Minor comments only.

@chriscollins3456 chriscollins3456 merged commit e12d910 into datahub-project:master Aug 24, 2023
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants