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.

Sign up for GitHub

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

Jump to bottom

[GH-4476] Bug: No card delete confirmation status for some cards #4612

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[GH-4476] Bug: No card delete confirmation status for some cards #4612

wants to merge 9 commits into from

Conversation

JesusMurguia
Copy link

Summary

I added checks for any content on the card (title, properties, comments, attachments, description).

Ticket Link

Fixes #4476

@mattermost-build
Copy link
Contributor

Hello @JesusMurguia,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

sbishel
sbishel requested changes Mar 2, 2023
View reviewed changes
Copy link
Member

@sbishel sbishel left a comment •
edited

Choose a reason for hiding this comment

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

Thanks, @JesusMurguia Let's create a common function for these to call to ensure consistency. It appears that the same changes are also necessary in galleryCard.tsx and fullCalendar.tsx

@JesusMurguia
Copy link
Author

JesusMurguia commented Mar 3, 2023 •
edited

Hello @sbishel, I noticed the fullCalendar.tsx logic is really different from the other files and I can't find a way to get the comments and attachments for the card because of the rules of hooks and stuff, I am afraid i don’t have much experience with that, could you give me some advice on that?

@sbishel
Copy link
Member

sbishel commented Mar 3, 2023

Hello @sbishel, I noticed the fullCalendar.tsx logic is really different from the other files and I can't find a way to get the comments and attachments for the card because of the rules of hooks and stuff, I am afraid i don’t have much experience with that, could you give me some advice on that?

Hi @JesusMurguia, sure I can try to provide some guidance. If you dig in to why the fullCalendar is different. It is essentially because the other uses (kanbanCard / galleryCard) are dealing with a single card. fullCalendar contains all the cards with logic for displaying individual cards.

I think the best (and probably easiest) way to handle this would be to create a similar component to kanbanCard...may-be calendarCard. Then the implementations would be able to be very similar to how it is handled in other places.

A further enhancement would then be to create a common card component that all the components could share. Not expecting you to do this, just pointing out that all three of these components will be very similar and could be handled by a common component.

sbishel
sbishel reviewed Mar 3, 2023
View reviewed changes
webapp/src/store/cards.ts Outdated
return getCards(state)[cardId]?.title === '' && getCards(state)[cardId]?.fields.contentOrder.length === 0 && Object.keys(getCards(state)[cardId]?.fields?.properties).length === 0 && !state.comments.commentsByCard[cardId] && !state.attachments.attachmentsByCard[cardId]
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggest using existing selectors to retrieve data vs accessing state directly.

Copy link
Author

Choose a reason for hiding this comment

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

I moved the isCardEmpty function into a hook and used the selectors there in my latest commit, please let me know if this is okay

@JesusMurguia
Copy link
Author

Got it, thank you. I will work on this

sbishel
sbishel reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Very nice...code looks good. Can you add unit tests where appropriate?

@sbishel sbishel requested a review from lindy65 March 6, 2023 23:29
@sbishel sbishel added 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester labels Mar 6, 2023
@JesusMurguia
Copy link
Author

I added the unit tests for the new component

@lindy65
Copy link

lindy65 commented Mar 7, 2023

Hi @JesusMurguia @sbishel

Please create a PR in mattermost server repo so I can create a test server to test this on?

Thank you :)

sbishel
sbishel approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Thanks @JesusMurguia. Nice work.

@sbishel
Copy link
Member

sbishel commented Mar 8, 2023

@lindy65 - I created a cloud server and uploaded the plugin built here (since we are back in plugin mode.) Normal cloud logins.

https://sbishel-4476.test.mattermost.cloud/

lindy65
lindy65 approved these changes Mar 13, 2023
View reviewed changes
Copy link

@lindy65 lindy65 left a comment

Choose a reason for hiding this comment

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

Thanks @JesusMurguia - this issue is fixed. I tested on the server Scott spun up and tested according to steps on #4476. Delete confirmation modal is present.
cc @sbishel

@lindy65 lindy65 added 3: Reviews Complete All reviewers have approved the pull request QA Verified Bug fix tested and verified by QA and removed 2: Dev Review Requires review by a core committer 2: QA Review Requires review by a QA tester labels Mar 13, 2023
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@sbishel
Copy link
Member

sbishel commented Mar 29, 2023

/update-branch

stypr added a commit to stypr/focalboard that referenced this pull request Sep 29, 2023
@stypr
[mattermostGH-4476] Bug: No card delete confirmation status for some…
b087eec 
… cards mattermost#4612
stypr added a commit to stypr/focalboard that referenced this pull request Sep 30, 2023
@stypr
Adding PR: Show card delete confirmation status properly (mattermost#…
106d89d 
…4612)
@Kuuchuu Kuuchuu mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbishel sbishel sbishel approved these changes

@lindy65 lindy65 lindy65 approved these changes

Assignees
No one assigned
Labels
3: Reviews Complete All reviewers have approved the pull request Contributor Lifecycle/1:stale QA Verified Bug fix tested and verified by QA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: No card delete confirmation status for some cards
4 participants
@JesusMurguia @mattermost-build @sbishel @lindy65