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

Notes 100: Comment Delete #1191

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

Conversation

OlegMoshkovich
Copy link
Member

@OlegMoshkovich OlegMoshkovich commented May 27, 2024

Copy link

netlify bot commented May 27, 2024

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit d37f1c8
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/667177af609e4600085a0dd7
😎 Deploy Preview https://deploy-preview-1191--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 16 (🔴 down 1 from production)
Accessibility: 91 (no change from production)
Best Practices: 92 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented May 27, 2024

Deploy Preview for bldrs-share-v2 ready!

Name Link
🔨 Latest commit df07f19
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share-v2/deploys/6657127eaaf1c8000843f6cd
😎 Deploy Preview https://deploy-preview-1191--bldrs-share-v2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@OlegMoshkovich
Copy link
Member Author

Blocked by PR #1167

@OlegMoshkovich OlegMoshkovich marked this pull request as ready for review May 30, 2024 11:24
@OlegMoshkovich OlegMoshkovich added blocked Extra attention is needed and removed blocked Extra attention is needed labels May 30, 2024
@OlegMoshkovich OlegMoshkovich marked this pull request as draft May 31, 2024 16:53
@OlegMoshkovich OlegMoshkovich marked this pull request as ready for review June 4, 2024 09:24
@@ -235,7 +235,7 @@ function githubHandlers(githubStore) {
rest.delete(`${GH_BASE}/repos/:org/:repo/issues/comments/:commentId`, (req, res, ctx) => {
const {org, repo, commentId} = req.params

if (org !== 'bldrs-ai' || repo !== 'Share' || !commentId) {
if (org !== 'pablo-mayrgundter' || repo !== 'Share' || !commentId) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move these to one of the mock test IDs to make it clear that it's not prod data, and also to free my ghost from the machine :)

@@ -14,7 +14,7 @@ describe('net/github/Comments', () => {
})

it('successfully delete comment', async () => {
const res = await deleteComment({orgName: 'bldrs-ai', name: 'Share'}, 1)
const res = await deleteComment({orgName: 'pablo-mayrgundter', name: 'Share'}, 1)
Copy link
Member

Choose a reason for hiding this comment

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

same

selected={showCreateComment}
onClick={selectCard}
icon={<AddCommentOutlinedIcon className='icon-share'/>}
/>
}
{editMode &&
<Box sx={{marginLeft: 'auto', padding: '0 0.5em'}}>
Copy link
Member

Choose a reason for hiding this comment

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

Could the left-padding be moved to the parent to remove it from have it repeated in these 3 components?

synched: (comment.id !== commentId) && comment.synched,
}))
async function deleteCommentGithub(commentId) {
await deleteComment(repository, commentId, accessToken)
Copy link
Member

Choose a reason for hiding this comment

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

I think we planned to use return statuses to set comments instead of filtering, like in #1200

Copy link
Member Author

Choose a reason for hiding this comment

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

@pablo-mayrgundter getting 204 with the data of the response object undefined.

ChatGPT:
A 204 No Content status code indicates that the server successfully processed the request, but is not returning any content. This is typical for a POST request where the server does not return a response body.

@OlegMoshkovich
Copy link
Member Author

Still working on the e2e tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Notes 100 - Comment Delete
2 participants