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(web-react): creates page to manage homepage posts #8292

Conversation

PatrickfBraz
Copy link
Contributor

@PatrickfBraz PatrickfBraz commented Jun 23, 2023

Context

Not long ago, the ads feature on the home page was added. It is an amazing feature to bring news to users. However, I believe that it has been very little explored due to the difficulties of managing the posts. The purpose of this PR is to aggregate everything already available in GraphQL in a page where users can more easily manage their posts on the home page.

Prints of the new page

New button (called "News") on homepage header to enter the Manage Posts page

image

Use the env variable "MANAGE_POSTS_ENABLED" in the .env file to enable/disable this page

Create new posts

image

The switch button controls which type of homepage post will be created (Link or Text). Users without permission to create posts can access the post creation modal, but an error will raise when click on create button.

Se your Posts

image

There is only option to delete your posts (yet..)

image

Confirming that the posts were created kkk

image

Caveats:

  • The option of appearing or not the button should be through platform privileges, but there is not yet a "Manage Homepage Posts" privilege.

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 the product PR or Issue related to the DataHub UI/UX label Jun 23, 2023
@anshbansal anshbansal added the community-contribution PR or Issue raised by member(s) of DataHub Community label Jun 24, 2023
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

nice! this is looking awesome - thank you so much for taking the time to do this!

The two main things I think that need to be done are:

  1. add a new platform permission for managing posts to control who can do this (and hide the tab in the UI if they can't)
  2. move the posts page from a button on the top to our Settings

BUILD_PATH=build/yarn
MANAGE_POSTS_ENABLED=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

what we've been trying to do is to create feature flags that are more global and can be set with env variables in our helm charts. this involves setting it in application.yaml with a default (true in this case) and passing that feature flag to the frontend in our app config graphql endpoint. An example PR doing exactly this and disabling a piece of the UI based on the flag is here: #8067

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm Thanks @chriscollins3456 !!

Comment on lines +7 to +15
// const SuggestedNamesGroup = styled.div`
// margin-top: 12px;
// `;
//
// const ClickableTag = styled(Tag)`
// :hover {
// cursor: pointer;
// }
// `;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you delete this commented out code?

Comment on lines +114 to +125
<LinkWrapper>
<Link to="/posts">
<Button id="manage-posts-page" type="text">
<Tooltip title="Manage home page posts">
<NavTitleContainer>
<NotificationOutlined />
<NavTitleText>News</NavTitleText>
</NavTitleContainer>
</Tooltip>
</Button>
</Link>
</LinkWrapper>
Copy link
Collaborator

Choose a reason for hiding this comment

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

after talking with our designer we have some concerns with adding another button to the top header of the app. Instead, we suggest putting this in Settings under the left sidebar "Manage" section right underneath the new Ownership Types section. then we can render the homepage posts there in settings

@@ -107,6 +110,20 @@ export function HeaderLinks(props: Props) {
</Link>
</LinkWrapper>
)}
{showManagePosts && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

another thing we're going to need is a new permission for users to be able to manage these posts. So maybe a simple platform privilege MANAGE_POSTS or something. An example would be like MANAGE_GLOSSARIES privilege. This way datahub users can gate who is actually updating their home page with these posts - so that means some more java work as well

/**
* Add an entry to the list posts cache.
*/
export const addToListPostCache = (client, newPost, pageSize) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for building these cache updating functions!

Comment on lines +27 to +47
export function PostTitleColumn() {
return (record: PostEntry) => (
<span data-testid={record.urn}>
<PostTitleContainer>
<Typography.Text>{record.title}</Typography.Text>
</PostTitleContainer>
</span>
);
}

export function PostDescriptionColumn() {
return (record: PostEntry) => (
<span>
<PostTitleContainer>
<Typography.Text>{record.description}</Typography.Text>
</PostTitleContainer>
</span>
);
}

export function PostContentTypeColumn() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

all of these look pretty much the same except for the data-testid added - could we make this a singular generic function? we could pass in an optional dataTestId as a prop if we wanted that and only wanted that on one of the column titles

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.

copy paste typo - should be deletePost

@chriscollins3456
Copy link
Collaborator

hey @PatrickfBraz thanks again for this! let me know if you want any assistance pushing this thing forward :)

@chriscollins3456
Copy link
Collaborator

So we actually had a need to get this feature in sooner than expected so I ended up taking your work here and building off of it/finished things up in my PR here: #8707

Thank you so much for your hard work on this! it's going to be such a nice feature that people will love.

@anshbansal
Copy link
Collaborator

Closing this PR as the work was done in another PR. Thanks for raising this.

@anshbansal anshbansal closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants