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

[FIX] Description Under Overview Action Showing null #39

Merged
merged 2 commits into from
Feb 19, 2023

Conversation

Nabhag8848
Copy link
Contributor

Issue(s)

Closes #38

Fixed Bugs

  • Removed redundant backticks around the description and tags under Overview of Repository
  • No description provided text instead of null when the repository doesn't have a description.

Screenshot from 2022-12-04 16-13-04

Copy link
Collaborator

@samad-yar-khan samad-yar-khan left a comment

Choose a reason for hiding this comment

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

@Nabhag8848 Thanks for your PR 🚀 Added some suggestions, can you please take a look ?

Comment on lines 67 to 72
let description: string | null = resData.description;

if(description === null){
description = 'No description provided.';
}

Copy link
Collaborator

@samad-yar-khan samad-yar-khan Feb 12, 2023

Choose a reason for hiding this comment

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

Hey @Nabhag8848 , thanks for solving this for us. Works well 🚀 !

But maybe we can do something like this
const description = resData.description || 'No description provided.';

This will make sure that the description is const and is not updated before sending it to the main channel. Also it fits with the code style we have in this file and saves some lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Nabhag8848 Can this PR be extended to fix some other places where we are showing null values ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! we can do that. Do you encounter any nulls, possibly i didn't encounter yet? if you have point me up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, @samad-yar-khan . I found lot of places where there is asyncronous operation without await and vice versa. should i make the commits within this PR? also we can merge my PR #41 branch with this one to make it bigger or we will include issue #47 within PR #41 later.
How should we proceed.

@samad-yar-khan
Copy link
Collaborator

@Nabhag8848 Can you please remove the last commit and make a separate Issue / PR to explain the position if await in calls since that does not fit the purpose of this PR?

github/GithubApp.ts Outdated Show resolved Hide resolved
@Nabhag8848
Copy link
Contributor Author

@Nabhag8848 Can you please remove the last commit and make a separate Issue / PR to explain the position if await in calls since that does not fit the purpose of this PR?

Thanks @samad-yar-khan and thanks for pointing out. done removing the same and learn the lesson, i should not commit changes without acknowledgement.

@samad-yar-khan
Copy link
Collaborator

@Nabhag8848 LGTM ! you can add the async-await changes in a different PR . Thanks for the PR !

@samad-yar-khan samad-yar-khan merged commit d2cc4ad into RocketChat:main Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Description Under Overview Action Showing null when Repo has No Description
2 participants